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

othttp: Allow dynamic span name based on URL Path or a functional option #616

Closed
marwan-at-work opened this issue Apr 4, 2020 · 3 comments · Fixed by #617
Closed

othttp: Allow dynamic span name based on URL Path or a functional option #616

marwan-at-work opened this issue Apr 4, 2020 · 3 comments · Fixed by #617
Milestone

Comments

@marwan-at-work
Copy link
Contributor

marwan-at-work commented Apr 4, 2020

Hi there 👋

I'm starting to replace OpenCensus Tracing with OpenTelemetry Tracing now that OT is in beta.

The first problem I'm running into is migrating ochttp into othttp

One thing I found a bit difficult to replicate is that othttp.NewHandler takes an operation argument which becomes the span name. In ochttp, the span name, by default, is the URL Path for every incoming request, or you can pass an option that formats the span name for every request here

However, for othttp, we don't have either of those options except the operation string argument which means for every route, I have to statically provide the span name for it.

This is mostly problematic when a user does not have control over http paths such as when using Twirp

Therefore, I see two solutions:

  1. A quick solution is to make it so that if a user passes an empty string such as othttp.NewHandler(myHandler, "") then the othttp Handler will default to taking the URL Path for each request as the Span Name.

  2. More ideally, I'd rather have the user provide a span formatter based on each request, similar to ochttp's FormatSpanName that was linked above. In othttp's use case, you can provide this as an option such as othttp.WithSpanName(func(r *http.Request) string), which might also prompt the removal of the operation string argument entirely.

@marwan-at-work marwan-at-work changed the title othttp: Allow dynamic span name based on URL Path othttp: Allow dynamic span name based on URL Path or a functional option Apr 4, 2020
@marwan-at-work
Copy link
Contributor Author

marwan-at-work commented Apr 4, 2020

For reference, I have 3 branches ready for a PR for both options:

  1. Option 1 PR https://github.com/marwan-at-work/opentelemetry-go/pull/new/616-1
  2. Option 2 PR https://github.com/marwan-at-work/opentelemetry-go/pull/new/616-2
  3. Option 2 but backwards compatible: https://github.com/marwan-at-work/opentelemetry-go/pull/new/616-3

@Aneurysm9
Copy link
Member

@marwan-at-work
Copy link
Contributor Author

Great, I opened a PR for it and I'm happy to adjust.

@pellared pellared added this to the untracked milestone Nov 8, 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 a pull request may close this issue.

3 participants