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

update trait Number<T> to Number #2109

Merged

Conversation

fraillt
Copy link
Contributor

@fraillt fraillt commented Sep 12, 2024

Changes

Simply updated trait Number<T> by removing unnecessary generic type from it.

There is exactly these changes (you can repeat it in 1 min)

  • search for regex Number(<[a-zA-Z0-9]*>) replace with Number
  • in trait Number replace T with Self.

What is the value? It's easier to understand to newcomers such as myself :)

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@fraillt fraillt requested a review from a team September 12, 2024 11:11
Copy link

linux-foundation-easycla bot commented Sep 12, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: fraillt / name: Mindaugas Vinkelis (06cf36f)

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.3%. Comparing base (b2e5640) to head (06cf36f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...-sdk/src/metrics/internal/exponential_histogram.rs 71.4% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2109   +/-   ##
=====================================
  Coverage   78.3%   78.3%           
=====================================
  Files        121     121           
  Lines      20815   20815           
=====================================
  Hits       16309   16309           
  Misses      4506    4506           

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

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks for another cleanup/simplification. The existing thing was probably done to meet some needs in the Histogram aggregation, not quite sure why.
This is still good to merge, and we can do another deep review/cleanup of Histograms.

(Histograms SUM should always be double, but looks like we have it as <T>. Good to check it further after this PR.)

@cijothomas
Copy link
Member

/easycla check

@cijothomas
Copy link
Member

/easycla

@cijothomas cijothomas closed this Sep 12, 2024
@cijothomas cijothomas reopened this Sep 12, 2024
@cijothomas
Copy link
Member

Looks like CLA is failing, similar to communitybridge/easycla#4329

@fraillt fraillt force-pushed the number-trait-doesnt-need-generic-type branch from 8121c3f to 06cf36f Compare September 12, 2024 17:35
@fraillt
Copy link
Contributor Author

fraillt commented Sep 12, 2024

Looks like CLA is failing, similar to communitybridge/easycla#4329

Sorry... I keep forgetting ;(

@fraillt
Copy link
Contributor Author

fraillt commented Sep 12, 2024

This is still good to merge, and we can do another deep review/cleanup of Histograms.

I'm actually looking at this right now :) it seams that both ExpoHistogram and Histogram could share some logic.
At the moment I see at least few issues:

  • ExpoHistogram - uses it's own implementation for attribute tracking, instead of utilizing ValueMap, and for this reason it doesn't have is_under_cardinality_limit check, and other improvements.
  • Histogram - uses ValueMap which is optimized for AtomicOperations, but histogram measurements cannot be performed in atomic maneir, fo this reason every measurement get's atleast to locks: one for attribute map, another for actually updating histogram aggregate.

So If I'll create a new revision soon to address this ;)

@cijothomas cijothomas merged commit 8bd3294 into open-telemetry:main Sep 12, 2024
24 of 25 checks passed
@cijothomas
Copy link
Member

ExponentialHistogram was left out from initial set of changes to leverage ValueMap, just to keep scope under control!
Yes if we can somehow avoid the dual lock (1st one is really a read lock) for histogram, that'd be awesome.

@cijothomas
Copy link
Member

Merging, as CLA issue is resolved. Other proposals looks good, happy to accept them via follow up PRs!

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.

3 participants