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

grpc/http: support custom KeyValues when reporting metrics #195

Closed
derekperkins opened this issue Aug 3, 2020 · 6 comments · Fixed by #306
Closed

grpc/http: support custom KeyValues when reporting metrics #195

derekperkins opened this issue Aug 3, 2020 · 6 comments · Fixed by #306
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request
Milestone

Comments

@derekperkins
Copy link

We've been running OpenCensus for ~2 years and are excited to move to OpenTelemetry. The standard API has served us well, but our biggest issue with some of the wrapper packages like ocgrpc and ochttp are that they don't support custom KeyValues (TagKeys) when recording metrics. I can register my own views that include them, but because of the way that they wrap my server, I'm not passing a context that contains said values. I need this on a per request basis, to report specific error types, latency per kv, etc.

Is that in scope for OpenTelemetry? Without it, I'm forced to copy and fork the implementation.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 4, 2020

Thanks for opening the issue. We're excited to include improvements as we try to make OpenTelemetry awesome for users. Can you provide a simplified code example that would describe what you are looking for? I'm not 100% sure I follow the ask here.

@derekperkins
Copy link
Author

derekperkins commented Aug 4, 2020

Essentially I want to be able to provide per request labels. I'll use OpenCensus code snippets since that's what I have, but it should provide enough info. I could add any tags to the context and they would then be available to any views that were measured with that context.

tagMutators := []tag.Mutator{
	tag.Insert(KeyCountry, country),
	tag.Insert(KeyLanguage, language),
	tag.Insert(KeyLocationType, locationType),
	tag.Insert(KeyProvider, provider),
}
newCtx, err := tag.New(ctx, tagMutators...)

The problem with ochttp for example is that it by default lives outside the lifespan of my context, so I can't provide any mutators in the context to it. I could invert the router, but typically I don't know the keys until I actually get into my route and pull things from the db, etc.

h := &ochttp.Handler{
	Handler:      mux,
	StartOptions: getTraceStartOptions(ctx),
}
h.ServeHTTP(w, r)

The problem is how to hoist my context back up to where the stats are being recorded. I've toyed with some various concepts around using interfaces on the response writer.

type TagMutator interface {
	TagMutators() []tag.Mutator
}

type TaggedResponseWriter struct {
	http.ResponseWriter
	tagMutators []tag.Mutator
}

func (w TaggedResponseWriter) TagMutators() []tag.Mutator {
	return w.tagMutators
}

That might be usable above this line
https://github.com/open-telemetry/opentelemetry-go/blob/master/instrumentation/othttp/handler.go#L186

tagMutator, ok := w.(TagMutator)
if ok {
	taggedCtx, err := tag.New(ctx, tagMutator.TagMutators()...)
	if err != nil {
		return err
	} else {
		ctx = taggedCtx
	}
}

h.valueRecorders[ServerLatency].Record(ctx, elapsedTime, labels...)

It's not a beautiful solution, but I think it illustrates my point. This lets me slice all the ochttp metrics with my own tags/keys without having to calculate and record the stats myself.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 5, 2020

@derekperkins thanks for the context, this seems like a reasonable ask. I'm interested to hear what @Aneurysm9 would propose as a solution for this. He is currently rewriting and migrating this instrumentation to the contrib repo (#190) and will have the most up to date understanding of the problem space I feel.

@MrAlias MrAlias transferred this issue from open-telemetry/opentelemetry-go Aug 6, 2020
@MrAlias MrAlias added area: instrumentation Related to an instrumentation package enhancement New feature or request labels Aug 6, 2020
@derekperkins
Copy link
Author

Any thoughts? I saw a related issue in the main ot-go repo.
open-telemetry/opentelemetry-go#1051

@Aneurysm9
Copy link
Member

My initial thought is perhaps to have the handler inject something into the request context that can be used to add labels:

// in instrumentation
labeler := &Labeler{}
ctx = injectLabeler(ctx, labeler)
h.handler.ServeHTTP(w, r.WithContext(ctx))

setAfterServeAttributes(span, bw.read, rww.written, rww.statusCode, bw.err, rww.err)

// Add request metrics

labels := append(labeler.labels, semconv.HTTPServerMetricAttributesFromHTTPRequest(h.operation, r))
...
// in handler
labeler := otelhttp.LabelerFromContext(r.Context())
labeler.Add(label.String("key", "value"))

I'll try to prototype something along these lines and see how it feels.

@derekperkins
Copy link
Author

@Aneurysm9 thanks for the response, that looks like a much nicer api than the one I proposed. It's simple to use, minimal overhead, looks great!

@MrAlias MrAlias added this to Needs Triage in GA Burndown Sep 3, 2020
@MrAlias MrAlias moved this from Needs Triage to In progress in GA Burndown Sep 3, 2020
@MrAlias MrAlias added this to the RC1 milestone Sep 3, 2020
GA Burndown automation moved this from In progress to Done Sep 9, 2020
@MrAlias MrAlias added this to Done in OpenTelemetry Go RC Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants