7 ms·

Piranha: An Open Source Tool to Automatically Delete Stale Code

88 points by vinnyglennon 6y ago

iandinwoodie 6y ago
Here is the link to the Piranha repo since I didn't see it mentioned in the article: https://github.com/uber/piranha https://github.com/uber/piranha
RickJWagner 6y ago
It sounds pretty fancy. I'd want a very thorough set of unit tests before trusting code to delete code, though.
- lazaroclapp 6y ago
  Piranha co-author here. I'd say you trust it a bit more than we do, then ;) As the paper and blog post mention, we use Piranha to create diffs/PRs against the original author of the code. Developers are expected to code-review the changes before they are ever landed. As used within Uber, Piranha won't ever auto-land code deletions. That still saves the time of: a) remembering that the flag is stale and must be removed, b) actually removing the code and putting the diff up. Piranha diffs for a single flag are also generally small enough to fully read them during code review. We do have comprehensive unit tests for both the Piranha tool and the target codebase(s), and the majority (65%) of diffs generated by Piranha are landed without changes (and the most common change in those that are changed is to delete extra lines that Piranha couldn't prove as stale). Thus far we haven't had an outage caused by a Piranha deletion, but certainly there have been incorrect diffs generated and caught either by CI or manual reviewers, requiring us to update the tool. We would not recommend landing diffs generated by Piranha - or other stale code removal tools - to master without any reviews right now :)
  - victords 6y ago
    Follow up question: In general, when a diff is generated, is the standard process to just do a manual code-review and CI, or do you also test it manually to check for unexpected side-effects?
    - mkr-plse 6y ago
      Manual code review and CI only. In our experience of deleting more than 2.5K flags, testing would have helped in one case but then the code was not tested well when the flag was introduced.
      - lazaroclapp 6y ago
        Just as an extra note: there are manual testing steps (and automated end-to-end test, and internal alpha test, etc), independent of whether the diff was Piranha- or developer-authored, but all those happen for the continuous delivery internal version of the app, which is after the diff has been landed to master, but before a release is sent to app stores. We would count an issue discovered there as an outage having made past "our tests", even though it could well be caught before it gets to any external users.
ublaze 6y ago
One of our internal experimentation systems handles this by capping rollouts to 95%. So the author is forced to clean up their code if they want a full rollout.
- sjtgraham 6y ago
  Brilliant. This reminds me of the apocryphal tale where NASA spent millions developing a pen that could work in microgravity… and Russia just used a pencil.
  - dean177 6y ago
    Please stop repeating this, it’s total nonsense.
    - smabie 6y ago
      That's why he said apocryphal..
      - dean177 6y ago
        Which means they are of doubtful origin. There is no doubt that this particular story is a complete fabrication
        jolux 6y ago
        Yup. Completely false. The Russians ended up using that pen too or their own version of it; the Americans started off using pencils but it turns out having electrically conductive graphite shavings floating around is bad for electronic equipment.
- obilgic 6y ago
  this eliminates the ability to do a revert though if things don't work as expected at %100
unnameduser1 6y ago
It relies heavily on feature flags. Makes sense for big systems with lots of feature flags. However it works for now only with Java, Swift, or Objective-C Code. Check it out if your projects fit those two conditions
clumsysmurf 6y ago
> Currently implemented for Objective-C, Swift, and Java programs Are Uber's Android apps written in Java? I would have thought they would be Kotlin.
- lazaroclapp 6y ago
  Most of the existing Android codebase is Java. There is some Kotlin code, and PiranhaKotlin is certainly in the roadmap somewhere but, especially when the goal is removing older features, Java makes the lion share of the use case for such a tool right now.
X6S1x6Okd1st 6y ago
Neat! I experimented with something similar and found the state of java AST transformers to be in a kind of painful state. I'll have to look though the code base to see what y'all used.
- lazaroclapp 6y ago
  We used Google's Error Prone framework. Pros: you get full compiler symbol/type information, in addition to the AST. Cons: you need to build the code to process it (Error Prone runs as a javac plugin), and if you want to do things efficiently then you are restricted to basically a single AST traversal. It's very use-case dependent whether you are better off just using e.g. javaparser.
ResidentSleeper 6y ago
A while ago I deleted my Uber/UberEats account (they didn't make it easy btw) because of how utterly unusable their apps were compared to competition. After me and my girlfriend spent 30 minutes trying to order food on 3 different platforms, which has been a repeated experience with them for years, I decided it's time to give up. Same obvious UI/UX bugs as over a year ago - I'm actually dumbfounded at how this is possible, do they even use their own app? To sum up, I tend to look at tech coming from Uber through a somewhat sarcastic lens these days.
- nyanpasu64 6y ago
  Try rooting your phone and installing a CPU monitor like https://f-droid.org/en/packages/com.eolwral.osmonitor/ https://f-droid.org/en/packages/com.eolwral.osmonitor/ . Then watch for apps burning CPU in the background. Uber might show up wink.
bluesign 6y ago
If I am not mistaken this tool requires technical debt to begin with. I think more important part is what caused this debt to begin with. In my opinion it lies in the implementation of feature flags.
- another-dave 6y ago
  Can you expand a bit on what you mean here? How would you introduce feature flags in a way that doesn't leave any debt behind in contrast to their approach? Or are you saying that feature flags themselves are inheritly debt-generating (which I think they acknowledge, but are taking it on prudently & deliberately) — in which case, what would you do instead?
  - bluesign 6y ago
    Actually what I meant was this tool is kind of a after thought of bad feature flag life cycle. When you set a feature flag, normally you should put a deadline on it, after this deadline, you should either choose to keep it, or remove it or you can extend the deadline. But seems like they accumulated a lot of flags, which stayed stale over time, and went unchecked and lack of feedback created the debt. Also unrelated but I prefer 2 stages for retirement of flags, from production and from code. From the example in the article. Instead of using direct access to: experiments.isTreated(RIDES_NEW_FEATURE) I think it is better to indirectly refer this from a function like (considering we are in context of RIDES module/class): function isNewFeatureEnabled(){ return experiments.isTreated(RIDES_NEW_FEATURE) } So when you want to retire this flag to disable new feature but want to keep code for some time (for few versions), but don't want to bloat your binary you can simply do: function isNewFeatureEnabled(){ return FALSE }
    - lazaroclapp 6y ago
      > When you set a feature flag, normally you should put a deadline on it, after this deadline, you should either choose to keep it, or remove it or you can extend the deadline. This is actually a feature we did identify as important to have in order to increase Piranha's effectiveness, and it is being added to our internal flag tracking, but it doesn't negate the need for the tool. An expiration date makes it easier for the tool to know when to run on a given feature (rather than using heuristics based on % rollout and days without changing the experiment), it still means that Piranha: a) reduces manual effort by auto-generating the (candidate) removal patch, and b) acts as the reminder portion for the expiration, so it's more actionable than just adding a task. The thing to note is that, even on a steady state where flags are removed as soon as they go stale, enough new experiments are being created every day that reducing the time spent cleaning them up is valuable. Also, you definitely don't want to block someone from fixing a crash because they have pending expired flags, so all you can really do with any expiration policy is to remind them. With Piranha, you are reminding them and reducing the friction to solve the issue. After all, the diff is right there for them to review and click 'land' on. As for the hardcoded flag value in your example above? What does that accomplish? It looks to me like you'd only be shipping dead code, since there is no runtime way of re-enabling the `RIDES_NEW_FEATURE` behavior (which is the main difference between "rolled 100%" vs "Piranha-removed"). It also makes harder to remove the related code later, since the semantic information about it being part of the feature is lost. If it's just about having the old code available, then version control does that already, no? What am I missing?
ginko 6y ago
Wouldn't "nibblefish" be a more apt name for this? https://en.wikipedia.org/wiki/Red_garra#Relationship_with_humans https://en.wikipedia.org/wiki/Red_garra#Relationship_with_hu...
abjecton 6y ago
So it's like Gilfoyle's anton but it only deletes stale code
awinter-py 6y ago
this seems to work by comparing feature flag configs I've always been a buyer of tools to make runtime stats available in the IDE would be awesome to overlay trace / route timing into the codebase on whatever part of the call graph is on the screen
ramraj07 6y ago
Can someone point to a good write-up on how to introduce and start using feature flags in a repo? Seems like I just can't find any good resources on this topic!
- sulZ 6y ago
  If you're looking to try it out in a simple way, you could probably dynamically load different functionality or modules with environment variables. But I'm also looking for a good resource
  - ramraj07 6y ago
    This is what we are trying now, but that can't do everything. Some teams use launch darkly, but I'm also looking at what level of abstraction we should try to do, and things such as testing both branches, etc. It seems an important architectural refactor to go into feature flag territory yet I see very little discussion on best practices there.
  - dabeeeenster 6y ago
    Hi - we're developing a 100% open source feature flagging platform that can achieve this - https://bullet-train.io/ https://bullet-train.io/ - would love feedback!
- yoz 6y ago
  I'm a Developer Advocate at LaunchDarkly, and we've written a bunch of different guides over the years. For a good intro that discusses first steps with flags and also how to approach testing flagged code, see our eBook "Effective Feature Management"[1] (Download page is right behind the form, you don't need to wait for an email or anything) One of the best ways to start is add a simple kill-switch to low-impact code. (More about that in the book.) But, I have to ask: what are the main issues that are blocking you from starting? Is it that you need ideas for cases, or are worried about breaking something, or just want to ensure best practices from the start? We're constantly writing new guides, both as blog posts[2] and long-lived guide articles on our documentation site[3]. Feel free to message me on Twitter[4] if you need more help! [1] https://launchdarkly.com/effective-feature-management-ebook/ https://launchdarkly.com/effective-feature-management-ebook/ [2] https://blog.launchdarkly.com/ https://blog.launchdarkly.com/ [3] https://docs.launchdarkly.com/guides https://docs.launchdarkly.com/guides [4] https://twitter.com/yoz https://twitter.com/yoz

88 points by vinnyglennon 6y ago

Here is the link to the Piranha repo since I didn't see it mentioned in the article: https://github.com/uber/piranha https://github.com/uber/piranha

RickJWagner 6y ago

It sounds pretty fancy. I'd want a very thorough set of unit tests before trusting code to delete code, though.

lazaroclapp 6y ago
Piranha co-author here. I'd say you trust it a bit more than we do, then ;) As the paper and blog post mention, we use Piranha to create diffs/PRs against the original author of the code. Developers are expected to code-review the changes before they are ever landed. As used within Uber, Piranha won't ever auto-land code deletions. That still saves the time of: a) remembering that the flag is stale and must be removed, b) actually removing the code and putting the diff up. Piranha diffs for a single flag are also generally small enough to fully read them during code review. We do have comprehensive unit tests for both the Piranha tool and the target codebase(s), and the majority (65%) of diffs generated by Piranha are landed without changes (and the most common change in those that are changed is to delete extra lines that Piranha couldn't prove as stale). Thus far we haven't had an outage caused by a Piranha deletion, but certainly there have been incorrect diffs generated and caught either by CI or manual reviewers, requiring us to update the tool. We would not recommend landing diffs generated by Piranha - or other stale code removal tools - to master without any reviews right now :)
- victords 6y ago
  Follow up question: In general, when a diff is generated, is the standard process to just do a manual code-review and CI, or do you also test it manually to check for unexpected side-effects?
  - mkr-plse 6y ago
    Manual code review and CI only. In our experience of deleting more than 2.5K flags, testing would have helped in one case but then the code was not tested well when the flag was introduced.
    - lazaroclapp 6y ago
      Just as an extra note: there are manual testing steps (and automated end-to-end test, and internal alpha test, etc), independent of whether the diff was Piranha- or developer-authored, but all those happen for the continuous delivery internal version of the app, which is after the diff has been landed to master, but before a release is sent to app stores. We would count an issue discovered there as an outage having made past "our tests", even though it could well be caught before it gets to any external users.

ublaze 6y ago

One of our internal experimentation systems handles this by capping rollouts to 95%. So the author is forced to clean up their code if they want a full rollout.

sjtgraham 6y ago
Brilliant. This reminds me of the apocryphal tale where NASA spent millions developing a pen that could work in microgravity… and Russia just used a pencil.
- dean177 6y ago
  Please stop repeating this, it’s total nonsense.
  - smabie 6y ago
    That's why he said apocryphal..
    - dean177 6y ago
      Which means they are of doubtful origin. There is no doubt that this particular story is a complete fabrication
      - jolux 6y ago
        Yup. Completely false. The Russians ended up using that pen too or their own version of it; the Americans started off using pencils but it turns out having electrically conductive graphite shavings floating around is bad for electronic equipment.
obilgic 6y ago
this eliminates the ability to do a revert though if things don't work as expected at %100

unnameduser1 6y ago

It relies heavily on feature flags. Makes sense for big systems with lots of feature flags. However it works for now only with Java, Swift, or Objective-C Code. Check it out if your projects fit those two conditions

clumsysmurf 6y ago

> Currently implemented for Objective-C, Swift, and Java programs Are Uber's Android apps written in Java? I would have thought they would be Kotlin.

lazaroclapp 6y ago
Most of the existing Android codebase is Java. There is some Kotlin code, and PiranhaKotlin is certainly in the roadmap somewhere but, especially when the goal is removing older features, Java makes the lion share of the use case for such a tool right now.

X6S1x6Okd1st 6y ago

Neat! I experimented with something similar and found the state of java AST transformers to be in a kind of painful state. I'll have to look though the code base to see what y'all used.

lazaroclapp 6y ago
We used Google's Error Prone framework. Pros: you get full compiler symbol/type information, in addition to the AST. Cons: you need to build the code to process it (Error Prone runs as a javac plugin), and if you want to do things efficiently then you are restricted to basically a single AST traversal. It's very use-case dependent whether you are better off just using e.g. javaparser.

ResidentSleeper 6y ago

A while ago I deleted my Uber/UberEats account (they didn't make it easy btw) because of how utterly unusable their apps were compared to competition. After me and my girlfriend spent 30 minutes trying to order food on 3 different platforms, which has been a repeated experience with them for years, I decided it's time to give up. Same obvious UI/UX bugs as over a year ago - I'm actually dumbfounded at how this is possible, do they even use their own app? To sum up, I tend to look at tech coming from Uber through a somewhat sarcastic lens these days.

nyanpasu64 6y ago
Try rooting your phone and installing a CPU monitor like https://f-droid.org/en/packages/com.eolwral.osmonitor/ https://f-droid.org/en/packages/com.eolwral.osmonitor/ . Then watch for apps burning CPU in the background. Uber might show up wink.

bluesign 6y ago

If I am not mistaken this tool requires technical debt to begin with. I think more important part is what caused this debt to begin with. In my opinion it lies in the implementation of feature flags.

another-dave 6y ago
Can you expand a bit on what you mean here? How would you introduce feature flags in a way that doesn't leave any debt behind in contrast to their approach? Or are you saying that feature flags themselves are inheritly debt-generating (which I think they acknowledge, but are taking it on prudently & deliberately) — in which case, what would you do instead?
- bluesign 6y ago
  Actually what I meant was this tool is kind of a after thought of bad feature flag life cycle. When you set a feature flag, normally you should put a deadline on it, after this deadline, you should either choose to keep it, or remove it or you can extend the deadline. But seems like they accumulated a lot of flags, which stayed stale over time, and went unchecked and lack of feedback created the debt. Also unrelated but I prefer 2 stages for retirement of flags, from production and from code. From the example in the article. Instead of using direct access to: experiments.isTreated(RIDES_NEW_FEATURE) I think it is better to indirectly refer this from a function like (considering we are in context of RIDES module/class): function isNewFeatureEnabled(){ return experiments.isTreated(RIDES_NEW_FEATURE) } So when you want to retire this flag to disable new feature but want to keep code for some time (for few versions), but don't want to bloat your binary you can simply do: function isNewFeatureEnabled(){ return FALSE }
  - lazaroclapp 6y ago
    > When you set a feature flag, normally you should put a deadline on it, after this deadline, you should either choose to keep it, or remove it or you can extend the deadline. This is actually a feature we did identify as important to have in order to increase Piranha's effectiveness, and it is being added to our internal flag tracking, but it doesn't negate the need for the tool. An expiration date makes it easier for the tool to know when to run on a given feature (rather than using heuristics based on % rollout and days without changing the experiment), it still means that Piranha: a) reduces manual effort by auto-generating the (candidate) removal patch, and b) acts as the reminder portion for the expiration, so it's more actionable than just adding a task. The thing to note is that, even on a steady state where flags are removed as soon as they go stale, enough new experiments are being created every day that reducing the time spent cleaning them up is valuable. Also, you definitely don't want to block someone from fixing a crash because they have pending expired flags, so all you can really do with any expiration policy is to remind them. With Piranha, you are reminding them and reducing the friction to solve the issue. After all, the diff is right there for them to review and click 'land' on. As for the hardcoded flag value in your example above? What does that accomplish? It looks to me like you'd only be shipping dead code, since there is no runtime way of re-enabling the `RIDES_NEW_FEATURE` behavior (which is the main difference between "rolled 100%" vs "Piranha-removed"). It also makes harder to remove the related code later, since the semantic information about it being part of the feature is lost. If it's just about having the old code available, then version control does that already, no? What am I missing?

ginko 6y ago

Wouldn't "nibblefish" be a more apt name for this? https://en.wikipedia.org/wiki/Red_garra#Relationship_with_humans https://en.wikipedia.org/wiki/Red_garra#Relationship_with_hu...

abjecton 6y ago

So it's like Gilfoyle's anton but it only deletes stale code

awinter-py 6y ago

this seems to work by comparing feature flag configs I've always been a buyer of tools to make runtime stats available in the IDE would be awesome to overlay trace / route timing into the codebase on whatever part of the call graph is on the screen

ramraj07 6y ago

Can someone point to a good write-up on how to introduce and start using feature flags in a repo? Seems like I just can't find any good resources on this topic!

sulZ 6y ago
If you're looking to try it out in a simple way, you could probably dynamically load different functionality or modules with environment variables. But I'm also looking for a good resource
- ramraj07 6y ago
  This is what we are trying now, but that can't do everything. Some teams use launch darkly, but I'm also looking at what level of abstraction we should try to do, and things such as testing both branches, etc. It seems an important architectural refactor to go into feature flag territory yet I see very little discussion on best practices there.
- dabeeeenster 6y ago
  Hi - we're developing a 100% open source feature flagging platform that can achieve this - https://bullet-train.io/ https://bullet-train.io/ - would love feedback!
yoz 6y ago
I'm a Developer Advocate at LaunchDarkly, and we've written a bunch of different guides over the years. For a good intro that discusses first steps with flags and also how to approach testing flagged code, see our eBook "Effective Feature Management"[1] (Download page is right behind the form, you don't need to wait for an email or anything) One of the best ways to start is add a simple kill-switch to low-impact code. (More about that in the book.) But, I have to ask: what are the main issues that are blocking you from starting? Is it that you need ideas for cases, or are worried about breaking something, or just want to ensure best practices from the start? We're constantly writing new guides, both as blog posts[2] and long-lived guide articles on our documentation site[3]. Feel free to message me on Twitter[4] if you need more help! [1] https://launchdarkly.com/effective-feature-management-ebook/ https://launchdarkly.com/effective-feature-management-ebook/ [2] https://blog.launchdarkly.com/ https://blog.launchdarkly.com/ [3] https://docs.launchdarkly.com/guides https://docs.launchdarkly.com/guides [4] https://twitter.com/yoz https://twitter.com/yoz