feat!: add spanProcessors property
#460
Conversation
pieh
left a comment
pieh
left a comment
There was a problem hiding this comment.
LGTM, left some NIT comments for you to consider, but I'm also fine with things as-is
| 'netlify.site.name': options.siteName, | ||
| }) | ||
|
|
||
| const spanProcessors = await Promise.all(options.spanProcessors ?? [await getBaseSpanProcessor()]) |
There was a problem hiding this comment.
Not requesting a change here, but just noting that we could skip default fallback and always demand consumer to configure things explicitly which might make it easier to understand what is being configured when checking consumers and not need to be aware there that this is a default.
Will leave decision up to you on this.
| }) | ||
| } | ||
|
|
||
| export const getBaseSpanProcessor = async (): Promise<SpanProcessor> => { |
There was a problem hiding this comment.
This is "base span processor" because of "legacy" behavior, but given the whole changes here are because some usage won't rely on it - then this no longer is a "base" one? Maybe naming could be more explicit about application of it - like getSystemLogJsonSpanProcessor or something like that.
Not a blocker, will leave decision up to you on this.
This is a bundle of changes to the
netlify/otelpackage to make it more flexible in different environments:Breaking: We stop looking for the
x-nf-enable-tracingheader and returning a no-op when it's present. I believe this should be a concern of the consumer, because we might want to use tracing in a context where HTTP headers don't even make sense.Introduce
spanProcessorsproperty: We currently expose anextraSpanProcessorsproperty which lets consumers add span processors on top of the default one (which outputs a JSON with the system log prefix). In some contexts, we might want to skip system logs entirely and just have a custom span processor in place. This is not currently possible. So I've introduced a newspanProcessorsproperty which is similar toextraSpanProcessorsexcept it does not include the base processor. I'm not in love with this approach, but couldn't find a better one. I'm open to suggestions.Export several OpenTelemetry types: When building a custom exporter, it's useful to be able to import things like
SimpleSpanProcessorfrom this module as opposed to having to install@opentelemetry/sdk-trace-nodein the consumer and ensure the version matches the one from@netlify/otel.