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

Dismantle AbstractInstrumentBuilder inheritance hierarchy #5820

Merged
merged 11 commits into from
Oct 13, 2023

Conversation

breedx-splk
Copy link
Contributor

Replaces #5772.

The use of inheritance for code reuse via AbstractInstrumentBuilder was a bitter pill to swallow. I think this cleans up considerably with some delegation and improved encapsulation.

I don't believe that this changes any public facing APIs.

I think that all of the Sdk{MetricType}Builder classes are just pure delegation now.

@breedx-splk breedx-splk requested a review from a team September 11, 2023 22:08
@breedx-splk breedx-splk changed the title Dismantle metric builders2 Dismantle AbstractInstrumentBuilder inheritance hierarchy Sep 11, 2023
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...o/opentelemetry/sdk/metrics/InstrumentBuilder.java 100.00% <100.00%> (ø)
...io/opentelemetry/sdk/metrics/SdkDoubleCounter.java 96.77% <92.30%> (-3.23%) ⬇️
...a/io/opentelemetry/sdk/metrics/SdkDoubleGauge.java 95.23% <90.00%> (-4.77%) ⬇️
.../opentelemetry/sdk/metrics/SdkDoubleHistogram.java 96.55% <90.00%> (-3.45%) ⬇️
...ntelemetry/sdk/metrics/SdkDoubleUpDownCounter.java 96.00% <92.30%> (-4.00%) ⬇️
...a/io/opentelemetry/sdk/metrics/SdkLongCounter.java 96.55% <90.90%> (-3.45%) ⬇️
...ava/io/opentelemetry/sdk/metrics/SdkLongGauge.java 95.65% <92.30%> (-4.35%) ⬇️
...io/opentelemetry/sdk/metrics/SdkLongHistogram.java 96.96% <91.66%> (-3.04%) ⬇️
...pentelemetry/sdk/metrics/SdkLongUpDownCounter.java 95.65% <90.90%> (-4.35%) ⬇️

... and 6 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

return this;
}

@Override
public String toString() {
Copy link
Member

Choose a reason for hiding this comment

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

Missing tests for all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting that we should have tests for toString() methods?

Copy link
Member

Choose a reason for hiding this comment

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

We do for some (most?) of the toString() methods; perhaps we should add them for the instrument builders as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I've traditionally been hesitant to test toString() because it's supposed to be purely informative and relying on toString() for real programmatic concerns is probably a bad, fragile practice.

In this case, I might also shy away from testing it because the implementation relies on the toString() of several other classes, including autovalue generated toString() from the InstrumentDescriptor. Is that really that helpful?

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Sorry for not getting to this sooner. It's been low priority for me since its just a refactor. But if you're willing to pick it back up I will respond promptly and work to get this approved / merged 🙂.

@jack-berg jack-berg merged commit 57d8334 into open-telemetry:main Oct 13, 2023
18 checks passed
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