8 ms·

The code review feature seems too expensive to run on every PR automatically (to me): $0.75 per 100 lines of code. From their example pricing: "if you have a ty

by ReidZB 7y ago

The code review feature seems too expensive to run on every PR automatically (to me): $0.75 per 100 lines of code. From their example pricing: "if you have a typical pull request with 500 lines of code, it would only cost $3.75 to run CodeGuru Reviewer on it." I wonder if it's actually good enough to justify that price.

Nican 7y ago
I was thinking about the same thing. Even for a small project of 3 developers, it seems like this would rise easily to the $100+/month, for suggestions that may not even be that useful.
- rstupek 7y ago
  at which point you'd stop paying for it I would imagine?
  - jtcruthers 7y ago
    There doesn't seem to be a point to start paying for it, and the time to stop paying for it seems mighty early
spyspy 7y ago
that's insanely expensive if you're doing any type of code generation.
- pc86 7y ago
  This seems like a good incentive not to be generating thousands of lines of codes with each PR, which most would probably consider a feature as opposed to a bug.
  - derision 7y ago
    exactly. IMO if you're generating code, it should happen at build/compile time not at checkin
    - scarejunba 7y ago
      What would the rationale be for that?
      - panda88888 7y ago
        IMO it’s because generates code is not “source code”. It’s more similar to object files—both are generated by running a compiler.
      - pc86 7y ago
        Generated code is an artifact of the source code. If you need it for something specific, regenerate it from the source when you pull that from your version control system. You're not getting any benefit by storing something that can be generated alongside the means to generate it.
        scarejunba 7y ago
        Thank you for your response. The advantages you get: * Hermetic builds are faster because code-gen only occurs when changes occur in the base code * Lots of docgen tools don't support incremental compilation * Diffs in generated code show up as diffs when you change the code-gen tool, easier to isolate changes that occur if your code-gen tool is upstream (say you want entire org on Thrift 0.9.2 from Thrift 0.8) Downsides I can see: * Large repo. * Source of truth is now the generated code, not the source, so someone else using the source could get a different result. Essentially acting in an empirical mode of operation (i.e. does it provide benefits for cost), and ignoring any philosophical objections, this seems like it could go either way depending on the situation.
        spyspy 7y ago
        > Source of truth is now the generated code, not the source, so someone else using the source could get a different result. This seems to apply to the other side, I think. Generating from source with different tooling or tool versions could create different results, whereas using the generated code guarantees consistent behavior.
  - Someone1234 7y ago
    If you split the same number of lines over two, three, ten, etc PRs it still costs the same. If anything it is incentivising code-golf via line minimization.
auslegung 7y ago
That’s incredibly cheap, assuming it provides good suggestions. How much time does it take you to review 500 lines of code change, and what’s your time worth? If it takes 10 minutes and your time is worth about $20/hour or more, this service will part for itself immediately.
- dajohnson89 7y ago
  You seem to assume that the code review tool can do everything that a human code reviewer can.
  - kerpele 7y ago
    On the other hand, a machine will not get tired or bored where a human most definitely will if the diff is anywhere near that 500 lines
    - scriptkiddy 7y ago
      I disagree. I regularly review PRs with more than 500 changed lines/20+ changed files. I read every single line. I put the same amount of effort into reviewing code as I do writing it; every software engineer should.
    - deleted 7y ago
      [deleted]
- ReidZB 7y ago
  I agree 100%: if it provides good enough suggestions, it could pay for itself pretty easily on regular day-to-day PRs. (Although: not all 500 line PRs are made equal.) My original comment was definitely unclear. I actually had two separate thoughts (that I didn't communicate well at all): (1) if your team has occasional large, automated PRs (code generation, automated refactors, etc), you probably don't want to run this tool on them because of cost, so anyone that has these large PRs and uses CodeGuru probably needs to build a way into their automation to suppress CodeGuru (or build a way to invoke it for specific PRs) (2) I also wonder if it's good enough to justify the price on regular PRs We don't have many situation (1) PRs where I work now, but they do come up occasionally. For example, I've used IntelliJ IDEA's structural find-and-replace to do very large automated refactors where CodeGuru would be very expensive and probably provide little value. We also do check in some generated code (we usually don't do this, but there are a couple exceptions where we weighed the tradeoffs and decided checking in the generated code was a better solution, in our eyes).
philshem 7y ago
“Time to hire some code golfers, we’ll put the whole app into one line of code.”
- mitchty 7y ago
  Nightmares in perl...
- skykooler 7y ago
  "CodeGuru says this should be separated into more lines..."
- bowmessage 7y ago
  Really though, why charge by-the-line on this kind of product? Imagine if CodeCommit or Lambda charged you by the line too!
deleted 7y ago
[deleted]
Aeolun 7y ago
That sounds pretty terrible to be honest. I cannot imagine getting that kind of value out of it (that I would not get with a simple linter).
tkahnoski 7y ago
Trying to compare it to another code analyzer... https://sonarcloud.io/about/pricing https://sonarcloud.io/about/pricing 100k lines for €10/mo.
- stingraycharles 7y ago
  Is that all code or just the diff, though? 100k lines of code in diffs seems like a lot, all code - not so much.
Eclyps 7y ago
Don't accidentally commit `node_modules` - that'd be a costly mistake!
- james_s_tayler 7y ago
  Or a 35,000 line XML configuration file.
  - ehsankia 7y ago
    Now that I think of it, if it's paid by lines of code, it perversely incentives people to minimize the lines of code, no? Does it count white space and comments? Can I minify my code before passing it to this, then unminify it?
    - bryanrasmussen 7y ago
      Many languages can have code written in them minimized down to a single line. I guess they must have a character count number equals a line qualifier somewhere.
      - ehsankia 7y ago
        But even then, still pushes people to shorten variable names and other kind of minification.
    - stefano 7y ago
      Can you just remove newlines from files? In most languages they're optional.
      - earenndil 7y ago
        Count semicolons then.
    - dizzy3gg 7y ago
      Maybe it’s on recommended line formatting
notjustanymike 7y ago
Or when a junior dev switches from tabs to spaces
- billman 7y ago
  heh.. I wish it were only junior devs.
- nivenhuh 7y ago
  Or when a junior dev switches from spaces to tabs ;)
femto113 7y ago
If it’s trained on software written by Amazon it’s probably worth the $3.75 just so you can do the exact opposite of what they recommend.
- avip 7y ago
  I concur that Amazon's engineers suck. Source: They have rejected me twice. Obviously have no clue what they're doing.
  - heyoni 7y ago
    Definitely no bias there ;P
- danpalmer 7y ago
  I don’t have much context, but I’ve never seen Amazon as a technical leader in the industry. They’re absolutely a business leader, and the services they provide can be good, but at a code level I’ve always thought of them as very MVP, if it works it’s good enough. For code review services I’d expect a level far above this. Maybe they are able to do that, but I don’t have any existing positive bias towards this, and a few things against it.
- JaRail 7y ago
  It'll probably generate irrelevant stats on the engineers to send directly to their managers to use against them in their next review.
- areactnativedev 7y ago
  You have no idea how much this resonated ^^ Just needded to add an AWS library in my code base and BAM! here is how my console will look on every reload from now on : :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/aws-sdk/lib/react-native-loader.js -> node_modules/aws-sdk/lib/credentials/temporary_credentials.js -> node_modules/aws-sdk/clients/sts.js -> node_modules/aws-sdk/lib/react-native-loader.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/aws-sdk/lib/react-native-loader.js -> node_modules/aws-sdk/lib/credentials/cognito_identity_credentials.js -> node_modules/aws-sdk/clients/cognitoidentity.js -> node_modules/aws-sdk/lib/react-native-loader.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:28851 Warning: AsyncStorage has been extracted from react-native core and will be removed in a future release. It can now be installed and imported from '@react-native-community/async-storage' instead of 'react-native'. See https://github.com/react-native-community/react-native-async-storage https://github.com/react-native-community/react-native-async... reactConsoleErrorHandler @ :8081/index.bundle?platform=ios&dev=true&minify=false:28851 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/analytics/lib/Providers/index.js -> node_modules/@aws-amplify/analytics/lib/Providers/AWSKinesisFirehoseProvider.js -> node_modules/@aws-amplify/analytics/lib/Providers/index.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/predictions/lib/types/Providers/AbstractConvertPredictionsProvider.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/index.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/AbstractConvertPredictionsProvider.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/predictions/lib/types/Providers/index.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/AbstractIdentifyPredictionsProvider.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/index.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/predictions/lib/types/Providers/index.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/AbstractInterpretPredictionsProvider.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/index.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/predictions/lib/Providers/index.js -> node_modules/@aws-amplify/predictions/lib/Providers/AmazonAIPredictionsProvider.js -> node_modules/@aws-amplify/predictions/lib/Providers/index.js
m0zg 7y ago
PR I sent yesterday, line changes: +2,703 −3,529. That's like 50 bucks just for that PR.
- aledalgrande 7y ago
  dude I hope that was the result of updating yarn.lock or similar, otherwise good luck to your reviewer!
  - m0zg 7y ago
    Nope, it's mostly code. It's a gnarly PR that atomically delivers a feature (and removes the feature that the new one replaces), but this looks about right for my weekly productivity overall, except I usually submit it in smaller PRs, and the delta is mostly lines added.
- perlgeek 7y ago
  If another developer reviewed your code, how much time it would it take them, and how much is that time worth? If you divide the 50 bucks by that number, you get a cost ratio. If it's lower than the ratio of (benefit expected by automatic code review) / (benefit expected by manual code review), it's worth using. I guess we can speculate all we want; in the end, only experience will show if the service is worth it or not.
binary_vitamin 7y ago
> I wonder if it's actually good enough to justify that price. If it can spot a lot of issues (performance, security, bug, etc), $3.75 is definitely a good deal to do it once a while but not on every single changes (e.g. fixing a typo in the code comment)
JaRail 7y ago
Tools like this should be built into your IDE. No developer ever wants automated feedback at the end of the process in a code review. There are lots of academic ML review/suggestion tools. Those people come to the table with trials and statistics to assess the quality of their results. Amazon probably copied one of those papers, added a rules-engine to recommend their own APIs, and slapped a hefty price tag on it.

by ReidZB 7y ago

Nican 7y ago

I was thinking about the same thing. Even for a small project of 3 developers, it seems like this would rise easily to the $100+/month, for suggestions that may not even be that useful.

rstupek 7y ago
at which point you'd stop paying for it I would imagine?
- jtcruthers 7y ago
  There doesn't seem to be a point to start paying for it, and the time to stop paying for it seems mighty early

spyspy 7y ago

that's insanely expensive if you're doing any type of code generation.

pc86 7y ago
This seems like a good incentive not to be generating thousands of lines of codes with each PR, which most would probably consider a feature as opposed to a bug.
- derision 7y ago
  exactly. IMO if you're generating code, it should happen at build/compile time not at checkin
  - scarejunba 7y ago
    What would the rationale be for that?
    - panda88888 7y ago
      IMO it’s because generates code is not “source code”. It’s more similar to object files—both are generated by running a compiler.
    - pc86 7y ago
      Generated code is an artifact of the source code. If you need it for something specific, regenerate it from the source when you pull that from your version control system. You're not getting any benefit by storing something that can be generated alongside the means to generate it.
      - scarejunba 7y ago
        Thank you for your response. The advantages you get: * Hermetic builds are faster because code-gen only occurs when changes occur in the base code * Lots of docgen tools don't support incremental compilation * Diffs in generated code show up as diffs when you change the code-gen tool, easier to isolate changes that occur if your code-gen tool is upstream (say you want entire org on Thrift 0.9.2 from Thrift 0.8) Downsides I can see: * Large repo. * Source of truth is now the generated code, not the source, so someone else using the source could get a different result. Essentially acting in an empirical mode of operation (i.e. does it provide benefits for cost), and ignoring any philosophical objections, this seems like it could go either way depending on the situation.
        spyspy 7y ago
        > Source of truth is now the generated code, not the source, so someone else using the source could get a different result. This seems to apply to the other side, I think. Generating from source with different tooling or tool versions could create different results, whereas using the generated code guarantees consistent behavior.
- Someone1234 7y ago
  If you split the same number of lines over two, three, ten, etc PRs it still costs the same. If anything it is incentivising code-golf via line minimization.

auslegung 7y ago

That’s incredibly cheap, assuming it provides good suggestions. How much time does it take you to review 500 lines of code change, and what’s your time worth? If it takes 10 minutes and your time is worth about $20/hour or more, this service will part for itself immediately.

dajohnson89 7y ago
You seem to assume that the code review tool can do everything that a human code reviewer can.
- kerpele 7y ago
  On the other hand, a machine will not get tired or bored where a human most definitely will if the diff is anywhere near that 500 lines
  - scriptkiddy 7y ago
    I disagree. I regularly review PRs with more than 500 changed lines/20+ changed files. I read every single line. I put the same amount of effort into reviewing code as I do writing it; every software engineer should.
  - deleted 7y ago
    [deleted]
ReidZB 7y ago
I agree 100%: if it provides good enough suggestions, it could pay for itself pretty easily on regular day-to-day PRs. (Although: not all 500 line PRs are made equal.) My original comment was definitely unclear. I actually had two separate thoughts (that I didn't communicate well at all): (1) if your team has occasional large, automated PRs (code generation, automated refactors, etc), you probably don't want to run this tool on them because of cost, so anyone that has these large PRs and uses CodeGuru probably needs to build a way into their automation to suppress CodeGuru (or build a way to invoke it for specific PRs) (2) I also wonder if it's good enough to justify the price on regular PRs We don't have many situation (1) PRs where I work now, but they do come up occasionally. For example, I've used IntelliJ IDEA's structural find-and-replace to do very large automated refactors where CodeGuru would be very expensive and probably provide little value. We also do check in some generated code (we usually don't do this, but there are a couple exceptions where we weighed the tradeoffs and decided checking in the generated code was a better solution, in our eyes).

philshem 7y ago

“Time to hire some code golfers, we’ll put the whole app into one line of code.”

mitchty 7y ago
Nightmares in perl...
skykooler 7y ago
"CodeGuru says this should be separated into more lines..."
bowmessage 7y ago
Really though, why charge by-the-line on this kind of product? Imagine if CodeCommit or Lambda charged you by the line too!

deleted 7y ago

[deleted]

Aeolun 7y ago

That sounds pretty terrible to be honest. I cannot imagine getting that kind of value out of it (that I would not get with a simple linter).

tkahnoski 7y ago

Trying to compare it to another code analyzer... https://sonarcloud.io/about/pricing https://sonarcloud.io/about/pricing 100k lines for €10/mo.

stingraycharles 7y ago
Is that all code or just the diff, though? 100k lines of code in diffs seems like a lot, all code - not so much.

Eclyps 7y ago

Don't accidentally commit `node_modules` - that'd be a costly mistake!

james_s_tayler 7y ago
Or a 35,000 line XML configuration file.
- ehsankia 7y ago
  Now that I think of it, if it's paid by lines of code, it perversely incentives people to minimize the lines of code, no? Does it count white space and comments? Can I minify my code before passing it to this, then unminify it?
  - bryanrasmussen 7y ago
    Many languages can have code written in them minimized down to a single line. I guess they must have a character count number equals a line qualifier somewhere.
    - ehsankia 7y ago
      But even then, still pushes people to shorten variable names and other kind of minification.
  - stefano 7y ago
    Can you just remove newlines from files? In most languages they're optional.
    - earenndil 7y ago
      Count semicolons then.
  - dizzy3gg 7y ago
    Maybe it’s on recommended line formatting

notjustanymike 7y ago

Or when a junior dev switches from tabs to spaces

billman 7y ago
heh.. I wish it were only junior devs.
nivenhuh 7y ago
Or when a junior dev switches from spaces to tabs ;)

femto113 7y ago

If it’s trained on software written by Amazon it’s probably worth the $3.75 just so you can do the exact opposite of what they recommend.

avip 7y ago
I concur that Amazon's engineers suck. Source: They have rejected me twice. Obviously have no clue what they're doing.
- heyoni 7y ago
  Definitely no bias there ;P
danpalmer 7y ago
I don’t have much context, but I’ve never seen Amazon as a technical leader in the industry. They’re absolutely a business leader, and the services they provide can be good, but at a code level I’ve always thought of them as very MVP, if it works it’s good enough. For code review services I’d expect a level far above this. Maybe they are able to do that, but I don’t have any existing positive bias towards this, and a few things against it.
JaRail 7y ago
It'll probably generate irrelevant stats on the engineers to send directly to their managers to use against them in their next review.
areactnativedev 7y ago
You have no idea how much this resonated ^^ Just needded to add an AWS library in my code base and BAM! here is how my console will look on every reload from now on : :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/aws-sdk/lib/react-native-loader.js -> node_modules/aws-sdk/lib/credentials/temporary_credentials.js -> node_modules/aws-sdk/clients/sts.js -> node_modules/aws-sdk/lib/react-native-loader.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/aws-sdk/lib/react-native-loader.js -> node_modules/aws-sdk/lib/credentials/cognito_identity_credentials.js -> node_modules/aws-sdk/clients/cognitoidentity.js -> node_modules/aws-sdk/lib/react-native-loader.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:28851 Warning: AsyncStorage has been extracted from react-native core and will be removed in a future release. It can now be installed and imported from '@react-native-community/async-storage' instead of 'react-native'. See https://github.com/react-native-community/react-native-async-storage https://github.com/react-native-community/react-native-async... reactConsoleErrorHandler @ :8081/index.bundle?platform=ios&dev=true&minify=false:28851 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/analytics/lib/Providers/index.js -> node_modules/@aws-amplify/analytics/lib/Providers/AWSKinesisFirehoseProvider.js -> node_modules/@aws-amplify/analytics/lib/Providers/index.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/predictions/lib/types/Providers/AbstractConvertPredictionsProvider.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/index.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/AbstractConvertPredictionsProvider.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/predictions/lib/types/Providers/index.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/AbstractIdentifyPredictionsProvider.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/index.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/predictions/lib/types/Providers/index.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/AbstractInterpretPredictionsProvider.js -> node_modules/@aws-amplify/predictions/lib/types/Providers/index.js Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle. metroRequire @ :8081/index.bundle?platform=ios&dev=true&minify=false:93 :8081/index.bundle?platform=ios&dev=true&minify=false:93 Require cycle: node_modules/@aws-amplify/predictions/lib/Providers/index.js -> node_modules/@aws-amplify/predictions/lib/Providers/AmazonAIPredictionsProvider.js -> node_modules/@aws-amplify/predictions/lib/Providers/index.js

m0zg 7y ago

PR I sent yesterday, line changes: +2,703 −3,529. That's like 50 bucks just for that PR.

aledalgrande 7y ago
dude I hope that was the result of updating yarn.lock or similar, otherwise good luck to your reviewer!
- m0zg 7y ago
  Nope, it's mostly code. It's a gnarly PR that atomically delivers a feature (and removes the feature that the new one replaces), but this looks about right for my weekly productivity overall, except I usually submit it in smaller PRs, and the delta is mostly lines added.
perlgeek 7y ago
If another developer reviewed your code, how much time it would it take them, and how much is that time worth? If you divide the 50 bucks by that number, you get a cost ratio. If it's lower than the ratio of (benefit expected by automatic code review) / (benefit expected by manual code review), it's worth using. I guess we can speculate all we want; in the end, only experience will show if the service is worth it or not.

binary_vitamin 7y ago

> I wonder if it's actually good enough to justify that price. If it can spot a lot of issues (performance, security, bug, etc), $3.75 is definitely a good deal to do it once a while but not on every single changes (e.g. fixing a typo in the code comment)

JaRail 7y ago

Tools like this should be built into your IDE. No developer ever wants automated feedback at the end of the process in a code review. There are lots of academic ML review/suggestion tools. Those people come to the table with trials and statistics to assess the quality of their results. Amazon probably copied one of those papers, added a rules-engine to recommend their own APIs, and slapped a hefty price tag on it.