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

Add purge method to TraceProvider #1380

Closed
wants to merge 1 commit into from

Conversation

thomasbeaujean
Copy link

Hi,
this PR to add the "purge" method to the traceProvider.

Use case

When the traces are activated on a project, all traces, without exceptions, are collected and sent to the span storage.

The storage is not free. I want to store only usefull data. When you process a lot of requests per second, It is quickly costly for nothing.

I am only interested with traces related to an incoming httpRequest related to a log with at least a level of warning.

In symfony, I did a POC that works:

  • use hook on LoggerInterface warning/error/... => set a static variable to true
  • use hook on HttpKernel, on "post", if variable true => forceFlush, and then purge, reset variable

With this, only >=warning related spans are sent.

The same system can be applied to messenger, etc.

The purge method allows this kind of behaviour without interfering with the original one.

@thomasbeaujean thomasbeaujean requested a review from a team September 18, 2024 07:11
Copy link

welcome bot commented Sep 18, 2024

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

@brettmc
Copy link
Collaborator

brettmc commented Sep 18, 2024

Hi @thomasbeaujean
That's not part of the spec, and so I'm hesitant to accept it into core (we can "go rogue", but would prefer not to if we can avoid it). However, this PR did remind me about a feature that was recently implemented, per-spec, which I have yet to document! Does #1353 help your use-case?

@thomasbeaujean
Copy link
Author

Hi @thomasbeaujean That's not part of the spec, and so I'm hesitant to accept it into core (we can "go rogue", but would prefer not to if we can avoid it). However, this PR did remind me about a feature that was recently implemented, per-spec, which I have yet to document! Does #1353 help your use-case?

I did not find anything about "purge" in the spec and I agree it is better to stick to the spec. I check the #1353 and the configuration allows to update the behaviour of trace provider.

If I get it right, the update of the behaviour does not match my use case. The SpanProcessor needs to collect all spans, and at the end of my incoming http request, my app decides to send or purge the spans. The decision is done afterward the collect of the spans.

The solution to destroy and recreates new Processors for each requests could be done but I feel that is would not be effective.

@brettmc
Copy link
Collaborator

brettmc commented Sep 18, 2024

If I get it right, the update of the behaviour does not match my use case. The SpanProcessor needs to collect all spans, and at the end of my incoming http request, my app decides to send or purge the spans.

This might be a use-case for the opentelemetry collector, then. It can discard entire spans after-the-fact (tail-based sampling), and locating the collector near to your application (eg sidecar), you can discard requests that you are not interested in.

Another point I would make about "purge", is that the batch span processor periodically exports collected spans, and so purging the queue will likely lead to broken traces being emitted since some of the spans will be emitted before you make the decision to purge the remainder.

@thomasbeaujean
Copy link
Author

If I get it right, the update of the behaviour does not match my use case. The SpanProcessor needs to collect all spans, and at the end of my incoming http request, my app decides to send or purge the spans.

This might be a use-case for the opentelemetry collector, then. It can discard entire spans after-the-fact (tail-based sampling), and locating the collector near to your application (eg sidecar), you can discard requests that you are not interested in.

Another point I would make about "purge", is that the batch span processor periodically exports collected spans, and so purging the queue will likely lead to broken traces being emitted since some of the spans will be emitted before you make the decision to purge the remainder.

I see in:
https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/tailsamplingprocessor#a-practical-example

  • Rule 5: Sample all traces if there is an error in any span in the trace.

So, my app send all traces to the collector, I use a property of the span to indicates if there was a warning+, and the collector, using the tailsamplingprocessor, based on this attribute decide to discard or not the spans.

It looks like it resolve my use case.

Thanks a lot for your help and your work!

I will try this and once I validate it is ok, I come back in this PR and close it if it is ok.

@thomasbeaujean
Copy link
Author

Hi, I used open-telemetry-contrib with tail sampling and it works.
So this PR has no use.

Thank you for your help.

@thomasbeaujean thomasbeaujean deleted the trace-purge branch September 19, 2024 15:29
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.

2 participants