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 initial support for tracing #32

Merged
merged 7 commits into from
Jul 29, 2021
Merged

feat: add initial support for tracing #32

merged 7 commits into from
Jul 29, 2021

Conversation

enocom
Copy link
Member

@enocom enocom commented Jul 23, 2021

This is what the Dial trace looks like:

image

This is what the Refresh trace looks like:

image

Some of the trace code is borrowed from here. Note: the code there uses context lookups to find the associated span, which has a performance penalty. This code uses a closure so callers don't need to keep track of the span.

Related to #15.

It's probably worth discussing how we want to name our traces, too, because they'll become part of the public API in effect. As a result, I've not used concrete type names for internal tracing, just so we can move the internals around without issue in the future.

@enocom enocom requested a review from kurtisvg July 23, 2021 19:44
@enocom enocom changed the title feat: add tracing for Dial, Connect, and Refresh feat: add tracing for Dial and Refresh with child spans Jul 23, 2021
Copy link
Contributor

@kurtisvg kurtisvg left a comment

Choose a reason for hiding this comment

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

One thing that aren't clear to me:

Are these exported all the time? What if someone wants to export somewhere specific (aka not Cloud Logging)?

dialer.go Show resolved Hide resolved
internal/trace/trace.go Outdated Show resolved Hide resolved
@enocom
Copy link
Member Author

enocom commented Jul 27, 2021

Are these exported all the time? What if someone wants to export somewhere specific (aka not Cloud Logging)?

The tracing is enabled only if a client has used OpenCensus to register an exporter. With no exporter registered, these traces become no-ops. Also, the client can choose their exporter as the tracing here is agnostic.

@enocom enocom requested a review from kurtisvg July 27, 2021 03:48
@kurtisvg
Copy link
Contributor

Are these exported all the time? What if someone wants to export somewhere specific (aka not Cloud Logging)?

The tracing is enabled only if a client has used OpenCensus to register an exporter. With no exporter registered, these traces become no-ops. Also, the client can choose their exporter as the tracing here is agnostic.

Let's include instructions in the README for this.

@kurtisvg kurtisvg changed the title feat: add tracing for Dial and Refresh with child spans feat: add initial support for tracing Jul 28, 2021
dialer.go Outdated Show resolved Hide resolved
@enocom
Copy link
Member Author

enocom commented Jul 28, 2021

I've added the instance name as an attribute to the parent span in both cases:

image

The naming scheme follows what I'm seeing from other labels, e.g., /http/user_agent, /http/host etc.

@enocom enocom requested a review from kurtisvg July 28, 2021 22:06
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