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

allow a user-defined function for providing span attributes on engine/connect span begin #2733

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

Conversation

jochs
Copy link

@jochs jochs commented Jul 24, 2024

Description

Add attrs_provider as an optional kwarg for SQLAlchemyInstrumentor().instrument. If attrs_provider (a 0 argument callable) is provided, it will be evaluated before any calls to start_current_span within the engine or connection instrumentation. This allows users who have an attribute aware sampler to inject attributes into the sampler to control sample behavior.

Specifically, for my usecase, I have a subclass of the ParentBasedTraceIdRatio called OverrideableParentBasedTraceIdRatio which allows passing of an 'x-ignore-sample' attribute which forces the sampler to emit a DROP decision when the parent is not already sampled. I would like to force all SqlAlchemy spans to be dropped unless the parent is sampled, which necessitates this change.

Fixes #2788

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • New test in instrumentation/opentelemetry-instrumentation-sqlalchemy/tests/test_sqlalchemy.py

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Copy link

linux-foundation-easycla bot commented Jul 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@jochs jochs requested a review from a team July 24, 2024 02:24
@jochs
Copy link
Author

jochs commented Jul 29, 2024

hey @shalevr - do you think you'd be able to review this sometime soon? Happy to create an issue if you need additional context

@lzchen
Copy link
Contributor

lzchen commented Aug 8, 2024

Specifically, for my usecase, I have a subclass of the ParentBasedTraceIdRatio called OverrideableParentBasedTraceIdRatio which allows passing of an 'x-ignore-sample' attribute which forces the sampler to emit a DROP decision when the parent is not already sampled. I would like to force all SqlAlchemy spans to be dropped unless the parent is sampled, which necessitates this change.

I'm a bit confused with the behavior of this custom sampler. Would you be able to provide a specific example? Can't you achieve this behavior with a custom ParentBased sampler?

@jochs
Copy link
Author

jochs commented Aug 27, 2024

hey @lzchen sorry I missed this. Yes, I am getting the behavior with a custom ParentBased sampler, but the sample decision depends on the attributes, and I currently can't provide attributes to the OTEL-sqlalchemy spans.

The sampler code looks like this:

class OverrideableParentBasedTraceIdRatio(ParentBasedTraceIdRatio):
    @override
    def should_sample(
        self,
        parent_context: Optional[Context],
        trace_id: int,
        name: str,
        kind: Optional[SpanKind] = None,
        attributes: Optional[Attributes] = None,
        links: Optional[Sequence[Link]] = None,
        trace_state: Optional[TraceState] = None,
    ) -> SamplingResult:
        if attributes is not None:
            if not not attributes.get(X_SAMPLED):
                return SamplingResult(
                    decision=Decision.RECORD_AND_SAMPLE,
                    attributes=attributes,
                    trace_state=_get_parent_trace_state(parent_context),
                )
            parent_span_context = get_current_span(parent_context).get_span_context()
            if not parent_span_context.trace_flags.sampled and not not attributes.get(X_IGNORE_SAMPLE):
                return SamplingResult(
                    decision=Decision.DROP,
                    attributes=attributes,
                    trace_state=_get_parent_trace_state(parent_context),
                )
        return super().should_sample(parent_context, trace_id, name, kind, attributes, links, trace_state)

And the idea is that I want to sample X% of the time in general but if I pass in the relevant attributes I can force record and sample or force drop. This is generally helpful for hot library code where I don't want to pay the overhead of tracing unless a span is already active via app code. Currently I'm somewhat blocked on sqlalchemy instrumentation because I don't want to pay that overhead cost on 5% of all sqlalchemy function calls (which is what the ratio is set at in production).

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.

Feature: ability to inject attributes into sqlalchemy spans to control sampling
4 participants