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

Sampling only Remote Traces #100

Merged
merged 7 commits into from
Feb 21, 2023
Merged

Sampling only Remote Traces #100

merged 7 commits into from
Feb 21, 2023

Conversation

renaz6
Copy link
Member

@renaz6 renaz6 commented Feb 17, 2023

  • By default, local traces are not sampled
  • Configuration added so that local tracing can be turned on if desired

@renaz6 renaz6 marked this pull request as draft February 17, 2023 22:52
@renaz6
Copy link
Member Author

renaz6 commented Feb 20, 2023

Documentation on Parent Based Sampling can be found here:https://pkg.go.dev/go.opentelemetry.io/otel/sdk/trace#ParentBased

sampler := sdktrace.ParentBased(sdktrace.NeverSample())

if sampleLocalTraces {
sampler = sdktrace.ParentBased(sdktrace.AlwaysSample())
Copy link
Member Author

@renaz6 renaz6 Feb 20, 2023

Choose a reason for hiding this comment

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

I'm not sure if we should use AlwaysSample() even if sampleLocalTraces is configured to be true. The documentation suggests not using this in production environments that see a lot of traffic, but I also doubt we'll see parent-less traces.
An alternative option is using TraceIDRatioBased() (https://pkg.go.dev/go.opentelemetry.io/otel/sdk/trace#TraceIDRatioBased) and only sending a percentage of traces through.

Copy link
Member

Choose a reason for hiding this comment

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

If we split apart the choices a bit more we can let the user decide what they want without us needing to do much & we can expand/grow later.

Replace sampleLocalTraces with:

  sampling-strategy:
    parent-initiated: ignore # [ignore (default), honor]
    no-parent: never         # [never (default), always, traceidratio, ...]
    traceidratio:            # For each sampler that needs inputs they get enumerated separately.
        percentage: 15.5

Choosing the quietest mode (completely off) as defaults.

Until we need to add traceidratio we can just know it goes here. I think @Sachin4403 may want:

  sampling-strategy:
    parent-initiated: honor
    no-parent: always

We'll probably want to use:

  sampling-strategy:
    parent-initiated: honor
    no-parent: never

We can add the traceidratio later if someone wants/needs it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Taking care of this in #102

@renaz6 renaz6 requested a review from denopink February 20, 2023 08:08
@renaz6 renaz6 marked this pull request as ready for review February 20, 2023 08:09
@renaz6 renaz6 self-assigned this Feb 20, 2023
@renaz6 renaz6 changed the title WIP: Sampling only Remote Traces Sampling only Remote Traces Feb 20, 2023
Copy link
Contributor

@johnabass johnabass left a comment

Choose a reason for hiding this comment

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

Given Wes's comment, we can do a separate PR to address the separate choices.

@sonarcloud
Copy link

sonarcloud bot commented Feb 21, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

85.7% 85.7% Coverage
22.2% 22.2% Duplication

@renaz6 renaz6 merged commit 218f87e into main Feb 21, 2023
@renaz6 renaz6 deleted the SampleRemoteTracesOnly branch February 21, 2023 18:53
This was referenced Feb 21, 2023
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.

None yet

4 participants