Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instruments with names which are case-insensitive equal contribute to… #5701

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

jack-berg
Copy link
Member

Resolves #5683.

Instruments with names which are case-insensitive equal contribute to same metric, advice is not part of instrument identity.

Embodies the comment here.

… same metric, advice is not part of instrument identity
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Patch coverage: 88.46% and project coverage change: +0.01% 🎉

Comparison is base (e42bd2f) 91.25% compared to head (b0b030b) 91.26%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5701      +/-   ##
============================================
+ Coverage     91.25%   91.26%   +0.01%     
- Complexity     5032     5042      +10     
============================================
  Files           556      556              
  Lines         14861    14901      +40     
  Branches       1389     1394       +5     
============================================
+ Hits          13562    13600      +38     
- Misses          896      899       +3     
+ Partials        403      402       -1     
Files Changed Coverage Δ
...lemetry/sdk/metrics/internal/state/DebugUtils.java 100.00% <ø> (ø)
.../metrics/internal/descriptor/MetricDescriptor.java 86.48% <81.81%> (-5.52%) ⬇️
...rics/internal/descriptor/InstrumentDescriptor.java 93.10% <92.00%> (-6.90%) ⬇️
.../metrics/internal/state/MetricStorageRegistry.java 96.15% <100.00%> (+3.56%) ⬆️
...metry/sdk/testing/assertj/LogRecordDataAssert.java 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just one comment about the test really. Thanks!

&& this.getDescription().equals(that.getDescription())
&& this.getView().equals(that.getView())
&& this.getSourceInstrument().equals(that.getSourceInstrument());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aggregationName used in the previous implementation is part of the view, so that's why it's not needed here. Just remarking because it gave me a small pause.

InstrumentValueType.DOUBLE,
Advice.empty()));
assertThat(descriptor1).isNotEqualTo(descriptor2).doesNotHaveSameHashCodeAs(descriptor2);
// Different instrument source type is not equal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each of these commented sections seems like it's actually testing for a separate thing. Why not make these into actually separate test methods then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this about the code style question of whether a single unit test method should test multiple cases or just a single one. (I feel like we've talked about this before 😄)

Personally, I do both depending on the situation, mostly depending on how much setup there is for each case. If the setup is small, then splitting each case out into its own method feels like unnecessary boiler plate on the author and reduced readability. On the other hand, if there's lots of special setup and you have to do things like reset the state of mocks, then it becomes fairly obvious to split out into separate methods. There's plenty of examples of both strategies in this repo. In this particular test, I was refactoring an existing test so stuck with the existing pattern. But even if it was new, I probably would have opted to keep all the not equals cases in a single test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] feels like unnecessary boiler plate on the author and reduced readability.

Until a test fails, and you have to dive into a line of code from a stack trace instead of quickly getting a meaningful test name that matches a method name.

I was refactoring an existing test so stuck with the existing pattern

Yeah, I respect the need to limit change range in a single PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until a test fails, and you have to dive into a line of code from a stack trace instead of quickly getting a meaningful test name that matches a method name.

Assertj relieves this with the ability to name / describe assertions. Didn't use it here tho, but probably should in cases where a single test method verifies multiple cases.

@jack-berg jack-berg force-pushed the fix-instrument-identity branch from 94d94cf to b0b030b Compare August 11, 2023 14:44
@jack-berg jack-berg merged commit 7ee92eb into open-telemetry:main Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix instrument identity evaluation in MetricStorageRegistry#register
4 participants