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

change tracing default to otlp #6351

Open
8 of 11 tasks
butonic opened this issue May 19, 2023 · 4 comments
Open
8 of 11 tasks

change tracing default to otlp #6351

butonic opened this issue May 19, 2023 · 4 comments
Labels
Category:Enhancement Add new functionality

Comments

@butonic
Copy link
Member

butonic commented May 19, 2023

jaeger now recommends using plain otlp so we should change our default config to the otlp endpoint localhost:4317.

For that we need to

to enable otlp tracing we can thens configure:

                "OCIS_TRACING_ENABLED":"true",
                "OCIS_TRACING_TYPE":"otlp",
                "OCIS_TRACING_ENDPOINT":"localhost:4317", // accept OpenTelemetry Protocol (OTLP) over gRPC, if enabled

Then we can go further:

I recommend using signoz to check the insights and metrics that can be deduced from the traces.

Further reading: https://opentelemetry.io/docs/concepts/instrumenting-library/

@ainmosni
Copy link
Contributor

ainmosni commented Jun 21, 2023

So, after a lot of debugging and discussions, we decided to redo the tracing setup following these notes:

  • One tracer span per service, not bothering with subspans yet.
  • One tracing package (in ocis-pkg?) that is the ONLY way to init a tracer.
    • Need to get tracer via New or similar function, no package global tracer to import at all.
    • In the New method you set all tracing configurables, like batching, sampling and what not.
  • ocis-pkg/tracing package also supplies config struct for tracing
  • For each service, tracer is configured at service init.
  • Implement own sampler where we can control if a certain trace gets sampled.
    • Sampler should check context for "should sample" param.
    • Sampler is percentage sampler for the rest.
  • Each service creates ONE tracer and their own grpc/http client.
    • If service already creates own client, needs propagation added.
  • Add WithTracer to reva

@micbar
Copy link
Contributor

micbar commented Jun 22, 2023

@ainmosni Thanks for the proposal.

I think we are pretty much aligned on the general approach. We should always put ourselves in the POV of our support engineers.

Examples

  1. We have a public link in production which fails only in 1 out of 100 times
  2. We get a request ID from the end user via his client log (Desktop, mobile or HAR from th browser)

@butonic
Copy link
Member Author

butonic commented Aug 25, 2023

New tasks

  • sampling should be configurable, see ideas in change tracing default to otlp  #6351 (comment)
  • reva uses a lot of global tracers and while we can pass a TracerProvider to the runtime not every place in reva uses it
  • reva should use per package Init() to initialize a tracer per package instead of a tracer string
  • uploads should persist the traceid in the uploadinfo so postprocessing can pick it up
  • kill all tracing options save otlp
  • figure out if we want to use a single tracer provider when running ocis in single binary mode. If not we have to pass the trace provider manually as in https://github.com/owncloud/ocis/pull/7113/files

Copy link

stale bot commented Dec 15, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status:Stale label Dec 15, 2023
@micbar micbar added the Category:Enhancement Add new functionality label Dec 15, 2023
@stale stale bot removed the Status:Stale label Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality
Projects
None yet
Development

No branches or pull requests

3 participants