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.

Sign up for GitHub

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

Jump to bottom

[SDK] Cardinality limits for metrics streams (Sync Instruments + Delta Temporality) #2255

Merged
merged 11 commits into from
Nov 21, 2023
Merged

[SDK] Cardinality limits for metrics streams (Sync Instruments + Delta Temporality) #2255

merged 11 commits into from
Nov 21, 2023

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Aug 4, 2023 •
edited
Loading

Changes

#2347

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team August 4, 2023 21:53
@codecov
Copy link

codecov bot commented Aug 4, 2023 •
edited
Loading

Codecov Report

Merging #2255 (c1569a7) into main (8cba762) will increase coverage by 0.01%.
The diff coverage is 84.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2255      +/-   ##
==========================================
+ Coverage   87.43%   87.43%   +0.01%     
==========================================
  Files         199      199              
  Lines        6028     6052      +24     
==========================================
+ Hits         5270     5291      +21     
- Misses        758      761       +3
Files Coverage Δ
...ntelemetry/sdk/metrics/state/sync_metric_storage.h 86.37% <100.00%> (ø)
...entelemetry/sdk/metrics/state/attributes_hashmap.h 92.43% <83.34%> (-2.81%) ⬇️

@github-actions github-actions bot added the Stale label Oct 7, 2023
@lalitb lalitb removed the Stale label Oct 9, 2023
@lalitb lalitb changed the title [WIP] Cardinality limits for metrics streams [WIP] Cardinality limits for metrics streams (Sync Instruments + Delta Temporality) Oct 9, 2023
@lalitb
Copy link
Member Author

lalitb commented Oct 9, 2023

This is ready for review now.

@esigo esigo self-assigned this Oct 9, 2023
@lalitb lalitb changed the title [WIP] Cardinality limits for metrics streams (Sync Instruments + Delta Temporality) Cardinality limits for metrics streams (Sync Instruments + Delta Temporality) Oct 9, 2023
marcalff
marcalff reviewed Oct 13, 2023
View reviewed changes
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Early review comments.

marcalff
marcalff reviewed Oct 13, 2023
View reviewed changes
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Overflow value

@lalitb
Copy link
Member Author

lalitb commented Nov 9, 2023

Thanks for the comments @marcalff. They are valid, have fixed them.

marcalff
marcalff approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

esigo
esigo approved these changes Nov 19, 2023
View reviewed changes
Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks :)

@esigo esigo added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Nov 20, 2023
@marcalff marcalff changed the title Cardinality limits for metrics streams (Sync Instruments + Delta Temporality) [SDK] Cardinality limits for metrics streams (Sync Instruments + Delta Temporality) Nov 21, 2023
@marcalff marcalff merged commit 9b89843 into open-telemetry:main Nov 21, 2023
45 checks passed
@marcalff marcalff mentioned this pull request Nov 21, 2023
Closed
@Porkepix Porkepix mentioned this pull request Dec 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esigo esigo esigo approved these changes

@marcalff marcalff marcalff approved these changes

Assignees

@esigo esigo

Labels
ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lalitb @marcalff @esigo