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

otelhttp: add service name #343

Merged
merged 1 commit into from
Oct 12, 2022
Merged

otelhttp: add service name #343

merged 1 commit into from
Oct 12, 2022

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Oct 12, 2022

Summary

This PR solves, hopefully, the last breaking change of OTEL default attributes behavioral change.

In the testnet dashboard, some tiles don't show information:
image

In the staging dashboard I deployed this PR and it shows data correctly.

Context

#323

Implementation overview

Luckily, after digging into the otelhttp, I saw that you can inject custom labels if you do an extra middleware wrapping.
It'll become clear while seeing the code.

Implementation details and review orientation

NA

@jsign jsign added the bug Something isn't working label Oct 12, 2022
@jsign jsign added this to the 5.0.1 milestone Oct 12, 2022
@jsign jsign self-assigned this Oct 12, 2022
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
}

func (lh *labeledHandler) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
labeler, _ := otelhttp.LabelerFromContext(r.Context())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The otelhttp.NewHandler in L13 creates a http.Handler that internally creates an internal &Labeler{}.

That means that in wrapped internal http.Handlers, middlewares can do otelhttp.LabelerFromContext(...) can get that "base labeler". That allows each handler to add custom extra attributes to be included in captured metrics.

This is exactly what we need. What we do is always include the service_name form the new metrics.BaseAttrs.

The same mechanics might be useful if we want to include custom labels in different handlers. For example, we could try including the chain_id, so we can have all the otelhttp metrics we have today, but also separated by chain_id, which we don't have.

We can think if that's useful in the future, it would be an easy addition. I'd have to just do both L22-L23-ish in the "chain id" detection middleware.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

@jsign jsign marked this pull request as ready for review October 12, 2022 17:58
@jsign jsign removed the request for review from asutula October 12, 2022 17:59
@jsign jsign added the dependencies Pull requests that update a dependency file label Oct 12, 2022
Copy link
Collaborator

@brunocalza brunocalza left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@jsign jsign merged commit 45217cb into main Oct 12, 2022
@jsign jsign deleted the jsign/fixm branch October 12, 2022 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants