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

Expand RecordingInstrucments to support collection of observers #112195

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Aug 26, 2024

The support is needed for RecordingInstruments to be used in tests for guages with a collection of observers.

Relates: #110630

The support is needed for RecordingInstruments to be used in tests for
guages with a collection of observers.

Relates: elastic#110630
@ywangd ywangd added >test Issues or PRs that are addressing/adding tests :Core/Infra/Metrics Metrics and metering infrastructure v8.16.0 labels Aug 26, 2024
@ywangd ywangd requested review from mosche and pxsalehi August 26, 2024 06:48
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Aug 26, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Contributor

@ldematte ldematte 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 adding this! A couple of thoughts:

  1. it seems the collection variants of registerLongsAsyncCounter and registerDoubleAsyncCounter are still not handled; can you add that?
  2. it also seems we have some duplication: RecordingDoubleCounter and RecordingAsyncDoubleCounter are identical. I think we can remove the second and just keep the former
  3. similarly, I think we don't need both RecordingAsyncLongCounter and RecordingLongCounter - it's probably better to change them to be like the double counter.

@ywangd
Copy link
Member Author

ywangd commented Aug 27, 2024

@ldematte Thanks for the review.

it seems the collection variants of registerLongsAsyncCounter and registerDoubleAsyncCounter are still not handled; can you add that?

Yep pushed b9c0bd0

it also seems we have some duplication: RecordingDoubleCounter and RecordingAsyncDoubleCounter are identical. I think we can remove the second and just keep the former

The two classes are quite different imo. The former is a synchronous counter with incrementXxx methods to actively publish metrics while the later is the synchronous version with callback to collect metrics. So I don't think we can remove either of them? Or do you mean RecordingDoubleGauge and RecordingAsyncDoubleCounter are fairly identical and you want only one of them? In this case, I'd prefer we keep them separate despite their implementations are mostly the same because they form different hierarchies of instruments. It feels rather confusing to me if we return the same class for a counter and a guage. It would also lead to many cascading changes to production code, e.g. APMMeterRegistry which uses related classes in method names and signatures.

@ywangd ywangd requested a review from ldematte August 27, 2024 02:03
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

LGTM; dont' worry about the (maybe) duplicated classes, I can follow up with some cleanup if needed. Thanks!

@ywangd ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 27, 2024
@elasticsearchmachine elasticsearchmachine merged commit d14fe77 into elastic:main Aug 27, 2024
16 checks passed
@ywangd ywangd deleted the augment-recording-meter-registry-for-observer-collection branch August 27, 2024 07:03
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Sep 4, 2024
…tic#112195)

The support is needed for RecordingInstruments to be used in tests for
guages with a collection of observers.

Relates: elastic#110630
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Metrics Metrics and metering infrastructure Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests test-update-serverless v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants