Initial OpenCensus Agent Exporter implementation

[justindsmith]

The OpenCensus Agent Exporter conforms to the Exporter interface, receiving
spans as they complete, adding them to a buffer, and sending the spans from
the buffer to the agent over a grpc stream.

OpenCensus proto files have been included, as well as types for the required
agent proto files. A series of adapters have been written to adapt from the
@opencensus/core internal span representation to the proto definitions.

Grpc streams are monitored for any disconnect and attempt to reconnect if
possible, utilizing a backoff strategy provided by the grpc client.

The exporter also listens to the Config stream, which recieves configuration
updates from the agent. Specifically, the current tracer sampler is updated
to either a ProbabilitySampler or ConstantSampler. The RateLimitingSampler
option is currently a no-op and has no effect on the tracer sampler.

[justindsmith]

This is the initial implementation of the agent exporter for issue #124. I will follow this up with a test suite (in this same PR) in a few days, but wanted to at least start the conversation. There are a few call out items here that I’d like some thoughts on.

/cc @kjin

1. The implementation copies the opencensus proto files into the repo and I’ve hand-crafted the types for the relevant protos for the agent work. I think it makes more sense for these to live in the census-instrumentation/opencensus-proto repository and to auto-generate the types but didn’t want to take that work on just yet. It would involve deploying a new package out of that repo. I don’t think it delays this PR and we can optimize it in a future release. It’s something we should look at though.

2. To handle the configuration updates from the agent, I’m getting a handle to the current tracer instance by loading the @opencensus/nodejs and getting the instance from there. That binds the exporter to nodejs implementation, which I think is fine, but wanted to call that out explicitly.

/cc @songy23

3. We are not supporting the stackTrace, sameProcessAsParentSpan, or childSpanCount properties today. The stackTrace isn’t supported by the core node implementation today. The sameProcessAsParentSpan I might be able to default to true (?) and the childSpanCount I can supply for the rootSpans but NOT for the child spans. Just let me know what you think is best with those.

4. I’d like some visibility into the hexStringToUint8Array() function in the adapters.ts file. Most of the adapters were straight forward, but since this is putting a value into a Buffer, I want to make sure that I’m handling that case properly (plus this is what we use to transport the trace/span id, so it’s pretty critical to get right).

5. In the core node implementation the MetricEvent.id is a string type, while in the agent type it’s a number. Looking at the code, the nodes implementation is creating a hex value, so I’m parsing that to an integer and sending that as the value, but that might be fragile. We might want to unify on the number type if that’s what’s expected.

6. Today we only support the createInsecure() credentials, does this need to be configurable by the user? Does the agent run in a secure way or can we assume we can always make an insecure connection?

I’m sure I’ll have more questions, but those should get us started!

[songy23]

Thanks @justindsmith for getting a PR ready in such a short time 👍

I think it makes more sense for these to live in the census-instrumentation/opencensus-proto repository and to auto-generate the types but didn’t want to take that work on just yet.

Make sense to me, please file a tracking issue on moving the gen-files to OpenCensus-Proto, similar to census-instrumentation/opencensus-python#318.

We are not supporting the stackTrace, sameProcessAsParentSpan, or childSpanCount properties today. The stackTrace isn’t supported by the core node implementation today.

Note that StackTrace is a deprecated concept and is not supported by any language (AFAIK). It's there just because of backwards-compatibility. See census-instrumentation/opencensus-go#912 (comment).

The sameProcessAsParentSpan I might be able to default to true (?) and the childSpanCount I can supply for the rootSpans but NOT for the child spans. Just let me know what you think is best with those.

FYI in Java data model, we have two optional fields hasRemoteParent and childSpanCount in SpanData, which correspond to sameProcessAsParentSpan and childSpanCount in proto. I think it's fine to leave a default value for them for now, but ultimately we should support both in the Node Span data model.

I’d like some visibility into the hexStringToUint8Array() function in the adapters.ts file. Most of the adapters were straight forward, but since this is putting a value into a Buffer, I want to make sure that I’m handling that case properly (plus this is what we use to transport the trace/span id, so it’s pretty critical to get right).

Defer the answer to @kjin since I'm not quite familiar with the Node encoding implementation.

In the core node implementation the MetricEvent.id is a string type, while in the agent type it’s a number. Looking at the code, the nodes implementation is creating a hex value, so I’m parsing that to an integer and sending that as the value, but that might be fragile. We might want to unify on the number type if that’s what’s expected.

Sorry I'm bit lost here - are you talking about MessageEvent? I didn't see there's a MetricEvent type in Node. If so, I agree the encoding should be consistent across language and we need to put it under OpenCensus-Specs. (Note that there was a PR about this but it was closed. /cc @bogdandrutu )

Today we only support the createInsecure() credentials, does this need to be configurable by the user? Does the agent run in a secure way or can we assume we can always make an insecure connection?

I think it makes sense to make it configurable by users. Note that in Go-Agent-Exporter we have an option for using insecure.

[justindsmith]

Make sense to me, please file a tracking issue on moving the gen-files to OpenCensus-Proto

Sure thing!

Sorry I'm bit lost here - are you talking about MessageEvent? I didn't see there's a MetricEvent type in Node. If so, I agree the encoding should be consistent across language and we need to put it under OpenCensus-Specs.

Oops! Yeah, I meant MessageEvent.id. Luckily I don't think the damage is too broad right now, so I'll try to make the change in this PR assuming @kjin is okay with it. (I didn't see anything in your linked PR about MessageEvent id type though - and a quick look through the specs didn't reveal anything either - but I don't think this is a huge change on the node side.)

I think it makes sense to make it configurable by users. Note that in Go-Agent-Exporter we have an option for using insecure.

Ok cool. I'll follow that model then.

I think it's fine to leave a default value for them for now, but ultimately we should support both in the Node Span data model.

I'll take a look at those implementations and see what I can do, but might leave them as default for the time being.

Thanks for the quick replies!

[songy23]

I didn't see anything in your linked PR about MessageEvent id type though - and a quick look through the specs didn't reveal anything either - but I don't think this is a huge change on the node side.

Oh yes that PR is only about Annotations. Anyway I've filed census-instrumentation/opencensus-specs#180 to follow up with adding cross-language specs on these fields.

I'll take a look at those implementations and see what I can do, but might leave them as default for the time being.

Sounds good to me :)

[justindsmith]

Updated the review to add unit tests and some other clean-up items. This PR is ready for a full review.

Here's from the commit:

• Add unit testing of exporter and adapters
• Ability to pass in a credentials to use for the grpc client connection.
• Move from grpc-tools to google-proto-files. The grpc-tools included a lot of unnecessary dependencies, we only need the google proto files.
• Added remoteParent to sameProcessAsParentSpan property, which flips the remoteParent boolean. Note that the remoteParent defaults to false and is NOT being set by any instrumentation today. // cc @songy23
• General cleanup.

[kjin]

Thanks for the contribution! Taking a look now

[kjin]

The implementation copies the opencensus proto files into the repo and I’ve hand-crafted the types for the relevant protos for the agent work. I think it makes more sense for these to live in the census-instrumentation/opencensus-proto repository and to auto-generate the types but didn’t want to take that work on just yet. It would involve deploying a new package out of that repo. I don’t think it delays this PR and we can optimize it in a future release. It’s something we should look at though.

I agree that this can happen as future work. Thanks for the heads up.

To handle the configuration updates from the agent, I’m getting a handle to the current tracer instance by loading the @opencensus/nodejs and getting the instance from there. That binds the exporter to nodejs implementation, which I think is fine, but wanted to call that out explicitly.

Sounds good. Sort of tangentially related, but I believe it would be better if a handle to the tracing object were moved to the core implementation (with the actual implementation being partly defined in @opencensus/nodejs, but this would be invisible to the user). Just a thought -- I haven't fully fleshed this out in my mind yet.

[kjin]

Generally looks great 👍 added a few suggestions.

[kjin]

LGTM