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

De-singleton ZPageServer implementation #4935

Merged
merged 5 commits into from
Nov 12, 2022

Conversation

breedx-splk
Copy link
Contributor

This addresses #1414.

I tried it with two instances and it seemed to work fine. I also included some simple manual span creation in the example for people to test with easily.

@breedx-splk breedx-splk requested a review from a team November 10, 2022 23:52
@jkwatson
Copy link
Contributor

nice! Thanks!

@codecov
Copy link

codecov bot commented Nov 11, 2022

Codecov Report

Base: 91.00% // Head: 90.98% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (4ecb895) compared to base (e814602).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4935      +/-   ##
============================================
- Coverage     91.00%   90.98%   -0.03%     
+ Complexity     4839     4838       -1     
============================================
  Files           546      546              
  Lines         14439    14439              
  Branches       1395     1395              
============================================
- Hits          13140    13137       -3     
- Misses          894      900       +6     
+ Partials        405      402       -3     
Impacted Files Coverage Δ
.../io/opentelemetry/api/internal/PercentEscaper.java 79.69% <0.00%> (-4.52%) ⬇️
...try/exporter/zipkin/ZipkinSpanExporterBuilder.java 97.72% <0.00%> (ø)
...exporter/jaeger/JaegerGrpcSpanExporterBuilder.java 80.64% <0.00%> (ø)
...metry/sdk/metrics/export/PeriodicMetricReader.java 90.00% <0.00%> (+2.85%) ⬆️
...ava/io/opentelemetry/sdk/internal/RateLimiter.java 100.00% <0.00%> (+5.88%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Just one minor comment

.setSpanLimits(getTracezTraceConfigSupplier())
.setSampler(getTracezSampler())
.build();
}
Copy link
Member

Choose a reason for hiding this comment

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

I would have this accept a SdkTracerProviderBuilder as an argument to make it more flexible.

Also, can we add test coverage for this method?

*
* @return new SdkTracerProvider
*/
public SdkTracerProvider buildSdkTracerProvider(SdkTracerProviderBuilder builder) {
Copy link
Member

Choose a reason for hiding this comment

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

I would have this return void, but don't feel strongly since its a convenience method and is experimental.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Yeah I totally see where you're coming from, but pure side-effect methods also give me the creeps. :)

@jack-berg jack-berg merged commit 61b1170 into open-telemetry:main Nov 12, 2022
dmarkwat pushed a commit to dmarkwat/opentelemetry-java that referenced this pull request Dec 30, 2022
* remove the static/singleton nature from ZPageServer

* add convenience method for creating zpages-based sdktracerprovider

* update readme for zpageserver

* oh checkstyle.

* overload build and add test
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