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.

Sign up for GitHub

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

Jump to bottom

Tracing: Add more detail to HTTP Outgoing Request #64757

Merged
merged 2 commits into from
Mar 27, 2023
Merged

Tracing: Add more detail to HTTP Outgoing Request #64757

merged 2 commits into from
Mar 27, 2023

Conversation

bboreham
Copy link
Contributor

What is this feature?

Adds more detail to the span for outgoing HTTP calls from Grafana backend.

image

Why do we need this feature?

This lets you see in the tracing view the relative contribution of each step - DNS request, establishing the connection, sending data, etc.

Who is this feature for?

Engineers investigating performance of Grafana and backend systems.

Which issue(s) does this PR fix?:

NA

Special notes for your reviewer:

OpenTelemetry by default will generate a bunch of sub-spans for DNS lookup, etc., but I prefer to have them as events, to save cluttering the view.

@bboreham
tracing: add more detail to HTTP Outgoing Request
e28c78d 
As events rather than sub-spans, to save cluttering the view.
@bboreham bboreham requested review from a team as code owners March 14, 2023 16:56
@bboreham bboreham requested review from wbrowne, xnyo, sakjur, suntala and yangkb09 and removed request for a team March 14, 2023 16:56
@CLAassistant
Copy link

CLAassistant commented Mar 14, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

wbrowne
wbrowne approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

LGTM

xnyo
xnyo approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

LGTM!

@wbrowne wbrowne changed the title tracing: add more detail to HTTP Outgoing Request Tracing: Add more detail to HTTP Outgoing Request Mar 15, 2023
@wbrowne
Copy link
Member

wbrowne commented Mar 15, 2023

@bboreham I assume this one is fine for 9.5.0? (IE doesn't need to roll out into a patch). Let me know if you need a hand with the labelling to satisfy the checks

@bboreham
Copy link
Contributor Author

No burning need to get it out, just something I thought would make an improvement.
I have no knowledge of grafana/grafana labeling, would welcome a pointer.

@bboreham bboreham added the no-backport Skip backport of PR label Mar 15, 2023
@bboreham
Copy link
Contributor Author

I found some instructions about labels in the 'Details' against CI checks.

@bboreham bboreham added this to the 9.5.0 milestone Mar 15, 2023
@bboreham
Copy link
Contributor Author

I just noticed it's also putting extra attributes in for every http header (with some redacted):
image

Personally I don't need that so I'll probably turn it off.

marefr
marefr approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@marefr marefr modified the milestone: 9.5.0 Mar 15, 2023
@bboreham bboreham marked this pull request as draft March 27, 2023 09:40
@bboreham bboreham marked this pull request as ready for review March 27, 2023 10:08
@bboreham
Copy link
Contributor Author

I have now pushed the commit turning off those extra attributes for headers.
Also note, although I work for Grafana Labs I am not authorized to merge to grafana/grafana myself, so someone else will have to merge this PR.

@marefr marefr merged commit 7731a4d into main Mar 27, 2023
@marefr marefr deleted the bboreham/httptrace branch March 27, 2023 11:04
@marefr marefr mentioned this pull request Mar 27, 2023
Closed
@xnyo xnyo mentioned this pull request Mar 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wbrowne wbrowne wbrowne approved these changes

@marefr marefr marefr approved these changes

@xnyo xnyo xnyo approved these changes

@sakjur sakjur Awaiting requested review from sakjur sakjur was automatically assigned from grafana/backend-platform

@suntala suntala Awaiting requested review from suntala suntala was automatically assigned from grafana/backend-platform

@yangkb09 yangkb09 Awaiting requested review from yangkb09 yangkb09 was automatically assigned from grafana/backend-platform

Assignees
No one assigned
Labels
add to changelog area/backend no-backport Skip backport of PR
Projects
None yet
Milestone
9.5.0
Development

Successfully merging this pull request may close these issues.
6 participants
@bboreham @CLAassistant @wbrowne @marefr @xnyo @grafanabot