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

Sample logs consistently with trace sampling #6814

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeanbisutti
Copy link
Member

The goal is to be able to sample logs consistently with trace sampling.

@jeanbisutti jeanbisutti requested a review from a team as a code owner October 22, 2024 15:03
@jeanbisutti jeanbisutti marked this pull request as draft October 22, 2024 15:03
@jeanbisutti
Copy link
Member Author

@jack-berg @jkwatson cc @trask

Would you agree to try to add this feature as experimental to this repository (perhaps with a different implementation)?

If the answer is yes, we could iterate in this repo:

  • See how to allow users to enable this feature
  • Add tests for log sampling
  • Discuss about the implementation
  • ...

Or would you recommend a different approach?

severityText,
body,
attributes));
SpanContext spanContext = Span.fromContext(context).getSpanContext();
Copy link
Member

Choose a reason for hiding this comment

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

This whole thing needs to be configurable.

For an approach consistent with tracing (which I think is a good baseline to consider), we need a new LogSampler interface analagous to Sampler.

The key should sample method might have a contract like:

boolean shouldSample(
      Context context,
      Severity severity,
      Value<?> body,
      Attributes attributes);

Notes:

  • Pass individual components of LogRecord instead of SdkReadWriteLogRecord so we can avoid allocations if shouldSample returns false. Also for consistency with tracing.
  • Omit resource, scope arguments for consistency with tracing
  • Omit timestamp, observed timesamp arguments for consistency with tracing
  • Omit severityText because it mostly duplicates severity
  • Return a boolean instead of the SamplingResult returned by tracing because log sampling is less complicated that trace sampling

With an interface like this, we could have built in samplers for common use cases. I.e. we could have a "trace based" sampler that retains logs if the span in context is sampled.

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