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

[config/confighttp] Resolve the TODO on confighttp.go related to prefixing operations with component id. #9382

Closed
atoulme opened this issue Jan 24, 2024 · 8 comments · Fixed by #12333
Assignees

Comments

@atoulme
Copy link
Contributor

atoulme commented Jan 24, 2024

From confighttp.go:

	// Enable OpenTelemetry observability plugin.
	// TODO: Consider to use component ID string as prefix for all the operations.
	handler = otelhttp.NewHandler(
		handler,
		"",
		otelhttp.WithTracerProvider(settings.TracerProvider),
		otelhttp.WithMeterProvider(settings.MeterProvider),
		otelhttp.WithPropagators(otel.GetTextMapPropagator()),
		otelhttp.WithSpanNameFormatter(func(operation string, r *http.Request) string {
			return r.URL.Path
		}),
	)

I have tried to resolve this TODO once before with #9123

@enesonus
Copy link

enesonus commented Jan 30, 2024

Hi @atoulme I would like to work on this issue. Is there any recommended readings or some files to look at? From what I understand, the span name formatter function can be used as a tool to set the prefix but I couldn't really understand how to get the component ID.

@atoulme
Copy link
Contributor Author

atoulme commented May 31, 2024

@enesonus go for it, not sure how to do it

@VihasMakwana
Copy link
Contributor

@atoulme can you assign this to me, I'll take it up.

@VihasMakwana VihasMakwana moved this from Todo to In Progress in Collector: v1 Sep 17, 2024
@VihasMakwana VihasMakwana moved this from In Progress to Waiting for reviews in Collector: v1 Sep 20, 2024
@mx-psi
Copy link
Member

mx-psi commented Feb 5, 2025

I think this can be removed from Collector v1, since we don't provide any stability guarantees for traces

@jade-guiton-dd
Copy link
Contributor

jade-guiton-dd commented Feb 10, 2025

What is the status of this issue @atoulme @VihasMakwana? I believe the scope is a bit unclear. Do we want to:

  1. make confighttp set the component ID as the span name prefix automatically; or,
  2. let users of confighttp set an arbitrary prefix for their component?

The original TODO seems to suggest 1, but this seems to have been abandoned in favor of 2 because the component ID is not available in the confighttp code.

If 2 is fine, then I believe this issue was already resolved by #11769, which allows users to use otelhttp.WithSpanNameFormatter themselves.

@atoulme
Copy link
Contributor Author

atoulme commented Feb 10, 2025

I opened this issue to remove this TODO and get the component to stable state. The TODO predates my time with the project. It's been thorny to resolve as everybody ends up guessing what the comment means in the context of the time it was written at. I believe you neatly address it in your comment. I also believe 2. addresses the need.

Can we please remove this TODO and close this issue? Would you like to open a PR?

@jade-guiton-dd
Copy link
Contributor

Alright, I'll do that.

github-merge-queue bot pushed a commit that referenced this issue Feb 10, 2025
#### Description
This PR deletes the TODO comment that sparked #9382. The original intent
is a bit unclear, but it is probably addressed by #11769. See tracking
issue for discussion and related PRs.

#### Link to tracking issue
Resolves #9382
@mx-psi mx-psi moved this to Done in Collector: v1 Feb 11, 2025
@jade-guiton-dd
Copy link
Contributor

FYI, I opened a PR (#12351) to add that prefix for the OTLP receiver (and other HTTP receivers will have to do the same independently).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment