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

feat: add tail sampling for traces #467

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Lp-Francois
Copy link

@Lp-Francois Lp-Francois commented Apr 17, 2024

Add tail sampling for traces. Useful when dropping health probes from your traces for instance, as they can be quite noisy 💸.

Documentation: https://grafana.com/docs/grafana-cloud/monitor-applications/application-observability/setup/sampling/tail/

Related issues:

Comments:

  • How to generate the example properly? I didn't find a way to generate the examples/tail-sampling/*.alloy files.
  • Is there an easy way of testing the generated config? I made sure the helm chart output looks OK (with helm template k8s-monitoring charts/k8s-monitoring -f examples/tail-sampling/values.yaml > examples/tail-sampling/output.yaml), but didn't tested it would work correctly.
    • If there is no official way, I guess I could copy paste from the helm chart output the generated config and pass it locally to the alloy cli?
  • The allowed configuration is quite a lot: https://grafana.com/docs/alloy/latest/reference/components/otelcol.processor.tail_sampling/#blocks I wonder if there would be an easy way to pass config for the policies? Maybe like the extraConfig is working, just let the user pass any string?

Note: It is really painful to work with these alloy_config/*.alloy.txt files 😅

@CLAassistant
Copy link

CLAassistant commented Apr 23, 2024

CLA assistant check
All committers have signed the CLA.

- also remove default value, as they have to be defined anyway

based on reviews from @t00mas
@Lp-Francois Lp-Francois requested a review from t00mas May 6, 2024 10:14
@Lp-Francois Lp-Francois requested a review from t00mas May 8, 2024 14:30
@Lp-Francois
Copy link
Author

@t00mas I applied your reviews :)

Could you have another look if you find some time please 🙏 ?

@t00mas
Copy link
Contributor

t00mas commented May 28, 2024

I'm OK with this, just take into account @rlankfo 's recommendation here #443 (comment)

@rlankfo
Copy link
Member

rlankfo commented May 28, 2024

I'd be concerned about adding the tail sampling processor at this stage of the pipeline. It won't be scalable -- to scale the tail sampling processor we recommend introducing a load balancing exporter in a separate layer that can route traces by trace ID. @petewall do you have any thoughts here?

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.

4 participants