Skip to content

toggleable tracing via SpanMethodOptions#25

Closed
wesleyvicthor wants to merge 2 commits into
XSAM:mainfrom
wesleyvicthor:trace_toggle
Closed

toggleable tracing via SpanMethodOptions#25
wesleyvicthor wants to merge 2 commits into
XSAM:mainfrom
wesleyvicthor:trace_toggle

Conversation

@wesleyvicthor

@wesleyvicthor wesleyvicthor commented Aug 28, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hi, this is a proposal to the issue #22

this PR allows toggling database operations. this leaves on
instrumentation config the decision of choosing which operation/method to trace.
this is an initial implementation and serves as presentation of a
possible approach to the functionality.

By default, operations must be explicitly configured

If you agree with the solution, I would move forward to add tests and implement the check for the remaining methods.
Any suggestion is welcome.

example

func main() {
	driverName, err := otelsql.Register("postgres",
		semconv.DBSystemPostgreSQL.Value.AsString(),
		otelsql.WithSpanOptions(otelsql.SpanOptions{
			DisableErrSkip: true,
		}),
		otelsql.WithSpanMethodOptions(
                         otelsql.MethodConnQuery,
                         otelsql.MethodConnExec,
                         otelsql.MethodRows,
                ),
	)
	if err != nil {
		panic(err)
	}
}

@wesleyvicthor
toggeable database operations
cfb304b 
this commit allows toggling database operations. this leaves on
instrumentation config the decision of choosing which operation/method to trace.
this is an initial implementation and serves as presentation of a
possible approach to the functionality
@wesleyvicthor
fix doc typo
ddbdc21
@wesleyvicthor
wesleyvicthor requested a review from XSAM as a code owner August 28, 2021 15:01
@XSAM

XSAM commented Aug 31, 2021

Copy link
Copy Markdown
Owner

@wesleyvicthor Thanks for raising this PR.

IIUC, what #22 wants is adding an option to disable the full query text of the executed query, which is populated in db.statement attribute.

This PR, which tries to control which DB operation will be recorded in the Span, seems not solving #22. If so, could you please create a new issue to discuss this SpanMethodOptions feature?

@wesleyvicthor

wesleyvicthor commented Aug 31, 2021

Copy link
Copy Markdown
Contributor Author

Hi @XSAM thanks for the clarifications, indeed I misinterpreted the desired feature. I assumed(as well considering my case) that a desirable behaviour would be to choose which db operation's span to record. So SpanMethodOptions configure that. If Register doesn't contain a SpanMethodOptions, with this implementation, no spans are recorded. I am aware this is by far not what we want. But my mindset here was to explicitly allow span configuration, however with a possible opposite version, e.g: WithoutSpanMethods, which given a list of methods, those wouldn't be traced.

By now I understand that our wish is for silencing semconv.DBStatementKey.String(query), however, still recording the span.

I will open another issue explaining SpanMethodOptions with details. But I believe this might not be wanted.

@wesleyvicthor wesleyvicthor changed the title toggleable database operations toggleable tracing via SpanMethodOptions Aug 31, 2021
@wesleyvicthor wesleyvicthor mentioned this pull request Aug 31, 2021
Merged
@seh

seh commented Sep 1, 2021

Copy link
Copy Markdown

This proposed design treats all "span method options" as boolean. If we're willing to write more verbose options-defining functions, we don't need to confine ourselves only to boolean options in the future. As stated, these options could just as well be a bitfield, since they're only enabling independent things presumed to be disabled by default.

@wesleyvicthor

wesleyvicthor commented Sep 1, 2021

Copy link
Copy Markdown
Contributor Author

Hi @seh indeed using bitwise operations could give more meaning to this feature. However, I believe this feature is not our target. I am closing this PR. Thanks for checking out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XSAM XSAM Awaiting requested review from XSAM XSAM is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wesleyvicthor @XSAM @seh