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

[service] add level for trace configuration #10892

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

codeboten
Copy link
Contributor

This allows users to configure a Noop TracerProvider if needed. Note that this configuration option breaks the zpages extension, which relies on a SDK implementation.

Fixes #10890

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.60%. Comparing base (d2ed276) to head (0c53567).
Report is 35 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10892   +/-   ##
=======================================
  Coverage   91.60%   91.60%           
=======================================
  Files         404      404           
  Lines       18990    18990           
=======================================
  Hits        17395    17395           
  Misses       1235     1235           
  Partials      360      360           

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

@codeboten codeboten force-pushed the codeboten/trace-level branch from 2b0321e to 7e30f5b Compare August 15, 2024 19:57
@codeboten codeboten marked this pull request as ready for review August 15, 2024 19:57
@codeboten codeboten requested review from a team and TylerHelmuth August 15, 2024 19:57
This allows users to configure a Noop TracerProvider if needed. Note that this
configuration option breaks the zpages extension, which relies on a SDK implementation.

Signed-off-by: Alex Boten <[email protected]>
@codeboten codeboten force-pushed the codeboten/trace-level branch from 84d2272 to 0c62d38 Compare August 16, 2024 15:36
@iblancasa
Copy link
Contributor

Please, can you elaborate a little bit more on this?

Note that this configuration option breaks the zpages extension, which relies on a SDK implementation.

@codeboten
Copy link
Contributor Author

Please, can you elaborate a little bit more on this?

Note that this configuration option breaks the zpages extension, which relies on a SDK implementation.

The zPages extension registers itself as a span processor to be able to record telemetry about all the spans emitted by the collector. This relies on the code here:

if ok {
sdktracer.RegisterSpanProcessor(zpe.zpagesSpanProcessor)
zPagesMux.Handle(path.Join("/debug", tracezPath), zpages.NewTracezHandler(zpe.zpagesSpanProcessor))
zpe.telemetry.Logger.Info("Registered zPages span processor on tracer provider")
} else {
zpe.telemetry.Logger.Warn("zPages span processor registration is not available")
}

That code has always worked with the default configuration because there was no way to disable the tracer provider until the introduction of the feature gate this change is replacing. With a no-op tracer provider, the zpages will be unable to register itself and log the following message: zPages span processor registration is not available

@iblancasa
Copy link
Contributor

Thanks a lot!

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

For the sake of consistency with MetricsConfig I understand why we're using Level here, but my mind just wants a boolean enabled option instead. I don't feel strongly enough to block this work.

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Some nits, LGTM otherwise

@@ -48,9 +49,10 @@ func attributes(set Settings, cfg Config) map[string]interface{} {

// New creates a new Telemetry from Config.
func newTracerProvider(ctx context.Context, set Settings, cfg Config) (trace.TracerProvider, error) {
if globalgates.NoopTracerProvider.IsEnabled() {
if globalgates.NoopTracerProvider.IsEnabled() || cfg.Traces.Level == configtelemetry.LevelNone {
Copy link
Member

Choose a reason for hiding this comment

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

Should it be documented somewhere that level:none breaks zpage?

Copy link
Contributor Author

@codeboten codeboten Aug 23, 2024

Choose a reason for hiding this comment

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

I can add a note in the zpages documentation, note that the zpages extension already logs a warning if this happens

.chloggen/codeboten_trace-level.yaml Show resolved Hide resolved
@codeboten codeboten merged commit b33913b into open-telemetry:main Aug 26, 2024
49 checks passed
@codeboten codeboten deleted the codeboten/trace-level branch August 26, 2024 19:19
@github-actions github-actions bot added this to the next release milestone Aug 26, 2024
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.

[telemetry] add service::telemetry::trace::level to allow enabling/disabling of tracer provider
4 participants