Skip to content
This repository has been archived by the owner on Nov 7, 2022. It is now read-only.

interceptor/opencensus: record number of received spans #65

Merged
merged 1 commit into from
Oct 7, 2018

Conversation

odeke-em
Copy link
Member

@odeke-em odeke-em commented Oct 5, 2018

Instrument interceptor/opencensus with stats to record
the number of received spans. This is accomplished by
a helper to process spans that sends over spans even
if the number of spans is 0. Also record with tag_key
"opencensus_interceptor" whose value is the name
of the respective interceptor.

The test to ensure that 0 length spans are also added
is currently disabled because:

Add tests to lock-in this behavior.

Updates #63

@odeke-em odeke-em requested a review from bogdandrutu October 5, 2018 01:19
@odeke-em odeke-em requested a review from songy23 October 5, 2018 01:19
@odeke-em odeke-em force-pushed the stats-instrument-opencensus-interceptor branch 2 times, most recently from f299e24 to 8993460 Compare October 5, 2018 01:23

// We MUST Unconditionally record metrics from this reception.

Choose a reason for hiding this comment

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

s/Unconditionally/unconditionally

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll update that.

@@ -88,20 +90,28 @@ func (oci *OCInterceptor) Export(tes agenttracepb.TraceService_ExportServer) err
return errTraceExportProtocolViolation
}

var lastNonNilNode *commonpb.Node
spansMetricsFn := internal.NewReceivedSpansRecorder(context.Background(), "opencensus")

Choose a reason for hiding this comment

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

Use tags from the current context (they include tags emitted by the oc-library-agent-exporter)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great reminder, that'd entail changing that to tes.Context(), thanks.

@bogdandrutu
Copy link

Probably forgot to push the commit. I cannot see any update.

@odeke-em odeke-em force-pushed the stats-instrument-opencensus-interceptor branch from f6cb1b9 to 2dc4ade Compare October 6, 2018 20:46
@odeke-em
Copy link
Member Author

odeke-em commented Oct 6, 2018

Probably forgot to push the commit. I cannot see any update.

@bogdandrutu in deed, PTAL!

@odeke-em odeke-em force-pushed the stats-instrument-opencensus-interceptor branch from 2dc4ade to 393b819 Compare October 7, 2018 02:53
@odeke-em odeke-em force-pushed the stats-instrument-opencensus-interceptor branch from 81c1aaf to e4c1aa3 Compare October 7, 2018 06:47
@odeke-em
Copy link
Member Author

odeke-em commented Oct 7, 2018

Thank you for the review @bogdandrutu!

Instrument interceptor/opencensus with stats to record
the number of received spans. This is accomplished by
a helper to process spans that sends over spans even
if the number of spans is 0. Also record with tag_key
"opencensus_interceptor" whose value is the name
of the respective interceptor.

The test to ensure that 0 length spans are also added
is currently disabled because:
* Issue census-instrumentation/opencensus-go#862
is not yet implemented which requests that the OpenCensus-Go
stats worker provide a method Flush to flush all data. Without
it, metrics will contain stale data from previous tests.

Add tests to lock-in this behavior.

Also replace grpc.NewServer with a stats enabled server helper call.
This enables tracing and metrics for each gRPC server by replacing
naked usages of grpc.NewServer with the new helper function:
    internal.GRPCServerWithObservabilityEnabled

Updates #63
@odeke-em odeke-em force-pushed the stats-instrument-opencensus-interceptor branch from e4c1aa3 to bdb4d3d Compare October 7, 2018 21:51
@odeke-em odeke-em merged commit bdb4d3d into master Oct 7, 2018
@odeke-em odeke-em deleted the stats-instrument-opencensus-interceptor branch October 7, 2018 22:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants