feat(trace): allow environment variables usage for url and serviceName #19
Conversation
vitalii-diachkov-sumup
force-pushed
the
default-url
branch
from
322459c to
0355467
Compare
There was a problem hiding this comment.
Thanks for this PR.
I'd like you to change a few things:
- keeping
service.nameattribute - keep exporter
urldefault value - check and display error in node status if there is no exporter URL configured
- keep
serviceNamedefault value - update node HTML documentation
- remove
mergeimport which is not used
lib/opentelemetry-node.js
Outdated
| CompositePropagator, | ||
| W3CBaggagePropagator, | ||
| W3CTraceContextPropagator, | ||
| merge, |
| parentSpan = tracer.startSpan(_rootPrefix + spanName, { | ||
| attributes: { | ||
| [ATTR_IS_MESSAGE_CREATION]: true, | ||
| [ATTR_SERVICE_NAME]: nodeDefinition.type, |
There was a problem hiding this comment.
I prefer to keep the service.name attribute for the parent, as it describes the logical name of the service (cf. https://opentelemetry.io/docs/specs/semconv/attributes-registry/service/).
It is different from node_red.node.type which is used to describe a specific Node-RED span attribute and code.function which is used for child spans only.
There was a problem hiding this comment.
When I tested it it was just set to http in, which confused me. Even if it contains the flow name itself, I don't want it to override service.name attribute on spans/traces, as service.name attribute for us is reserved for the name of internal microservice and is stattic across all application.
I can make it as another config option which is turned on by default, what do you think?
There was a problem hiding this comment.
For my understanding, you have a service name for each Node-RED instance?
In my use, I want to have the "parent" span telling me "yeah this is a message inject" (coming from inside my Node-RED) or "this trace come from an external http request".
lib/opentelemetry-node.js
Outdated
|
|
||
| // create tracer | ||
| let spanProcessor | ||
| const traceExporterProps = url ? { url } : {} |
There was a problem hiding this comment.
That's weird... exporting to nothing? 😆
Cf. previous comment about error prone.
There was a problem hiding this comment.
If URL is not explicitly provided, OTEL will derive the value from the environment. I can leave a code comment above this line
There was a problem hiding this comment.
Yep, add an explicit comment, and may be this is the place to add the check from previous comment.
lib/opentelemetry-node.html
Outdated
| value: '', | ||
| required: false, |
There was a problem hiding this comment.
It's also a source of errors (if it's left empty and there's no environment variable, traces come from unknown_service:/usr/local/bin/node)...
Ok not to be mandatory, but I prefer to keep the default value.
There was a problem hiding this comment.
I just thought since we have a placeholders, users might already have a clue on what's the expected value there. I can put the default there in place, also add a red badge when its missing (same as for URL)
There was a problem hiding this comment.
You can go checking in repo issues, sometimes users install without reading the doc and don't think about the configuration either and come telling "that's do not work" 😆
2 participants
As it was discussed in other issue before, OTEL allows you to configure your exporter URL and other attributes using environment variables, e.g.
OTEL_EXPORTER_ENDPOINTfor settings URL and service name can be set viaOTEL_SERVICE_NAMEor usingOTEL_RESURCE_ATTRIBUTES="service.name=my-service-name". This PR makesurlandserviceNamefields non-mandatory to set when configuringOTELnode.I've also noticed that child spans has different
service.namevalue as it was overwritten and set to the node name itself, for examplehttp request, which I found as unintuitive and undesired behaviour. If you want to keep this behaviour, I can remove this one change from the PR