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

Adds SpanFilter as a configuration option #174

Merged
merged 1 commit into from
Jun 11, 2023
Merged

Adds SpanFilter as a configuration option #174

merged 1 commit into from
Jun 11, 2023

Conversation

fantapop
Copy link
Contributor

@fantapop fantapop commented Jun 2, 2023

SpanFilter is a custom function that can be passed via configuration to control whether any given span should be included or omitted. (#169)

Example usage:

otelsql.SpanOptions{
    // Skip creating spans which don't have a parent
    SpanFilter: func(ctx context.Context, method otelsql.Method, query string, args []driver.NamedValue) bool {
        span := trace.SpanFromContext(ctx)
        return span.SpanContext().IsValid()
    },
}

@fantapop fantapop requested a review from XSAM as a code owner June 2, 2023 20:22
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Patch coverage: 100.0% and project coverage change: +0.5 🎉

Comparison is base (2956a1e) 80.0% compared to head (d5d3bd3) 80.5%.

❗ Current head d5d3bd3 differs from pull request most recent head dec9ce3. Consider uploading reports for the commit dec9ce3 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #174     +/-   ##
=======================================
+ Coverage   80.0%   80.5%   +0.5%     
=======================================
  Files         13      13             
  Lines        698     716     +18     
=======================================
+ Hits         559     577     +18     
  Misses       115     115             
  Partials      24      24             
Impacted Files Coverage Δ
config.go 88.8% <ø> (ø)
driver.go 94.7% <ø> (ø)
conn.go 78.2% <100.0%> (+0.8%) ⬆️
connector.go 100.0% <100.0%> (ø)
rows.go 54.5% <100.0%> (ø)
sql.go 80.9% <100.0%> (ø)
stmt.go 77.1% <100.0%> (+2.5%) ⬆️
tx.go 100.0% <100.0%> (ø)
utils.go 95.0% <100.0%> (+0.1%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

@fantapop Thank you for raising this PR and for the quality work! 👍🏽

I am still reviewing test cases, and it could take some time.
Some comments might need to be solved.

@fantapop fantapop changed the title Adds SpanOmitter as a configuration option Adds SpanFilter as a configuration option Jun 3, 2023
@fantapop fantapop force-pushed the main branch 2 times, most recently from 60c887a to e2fce7c Compare June 3, 2023 00:12
@fantapop
Copy link
Contributor Author

fantapop commented Jun 3, 2023

I pushed some fixup commits. I'll squash those into the one commit before merging. I've left some usage of the term omit within the code base since it was already existing and seems to be clearer to me whats going on. Let me know if you want me to change it.

@fantapop
Copy link
Contributor Author

fantapop commented Jun 8, 2023

Hi, have you had a chance to further vet this PR?

Copy link
Owner

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

LGTM! Comments might need to be resolved.

SpanFilter is a custom function that can be passed via configuration to
control whether any given span should be included or omitted.

Example usage:

otelsql.SpanOptions{
    // Only create spans if there is already a parent span
    SpanFilter: func(ctx context.Context, method otelsql.Method, query string, args []driver.NamedValue) bool {
        span := trace.SpanFromContext(ctx)
        return span.SpanContext().IsValid()
    },
}
@XSAM XSAM merged commit 807fc78 into XSAM:main Jun 11, 2023
@XSAM
Copy link
Owner

XSAM commented Jun 11, 2023

@fantapop Thanks for your contribution! 👍🏽

@XSAM XSAM mentioned this pull request Sep 8, 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

2 participants