feat: add log correlation/injection #121
Conversation
src/instrumentations/logging.ts
Outdated
| } | ||
|
|
||
| const logHook = (_span: Span, record: LogRecord) => { | ||
| record['service'] = options.serviceName; |
There was a problem hiding this comment.
any reason to not have this in the instrumentation packages? Otel spec recommends correlating with resources https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/overview.md#log-correlation
There was a problem hiding this comment.
AFAIK currently it's not possible to access the provider's resource information, or even reading span attributes; at least I haven't found any.
For reference:
https://open-telemetry.github.io/opentelemetry-js/interfaces/span.html
https://open-telemetry.github.io/opentelemetry-js/interfaces/tracerprovider.html
If that is the case I guess we need changes to the API to read this information, which might be useful for log correlation.
Edit: Taavo mentioned casting to BasicTracerProvider which the logger instrumentations could do, but not sure it can get through review at OTel. Might give it a try, but so long we can use this hook.
There was a problem hiding this comment.
Another option is to allow passing a resource directly to the instrumentation constructor. Users can do it manually and our distributions can do it automatically. Issue with this is that a single node project might want to use two different resources to represent sub-components which wouldn't work in this case.
There was a problem hiding this comment.
Updated it to read from the SDK span resource attributes.
Codecov Report
@@ Coverage Diff @@
## main #121 +/- ##
==========================================
+ Coverage 85.77% 85.82% +0.04%
==========================================
Files 11 12 +1
Lines 225 268 +43
Branches 46 61 +15
==========================================
+ Hits 193 230 +37
- Misses 32 38 +6
Continue to review full report at Codecov.
|
| resource: new EnvResourceDetector().detect(), | ||
| resource: new EnvResourceDetector().detect().merge( | ||
| new Resource({ | ||
| [ResourceAttributes.SERVICE_NAME]: options.serviceName, |
There was a problem hiding this comment.
If service name is not provided via options but is provided with OTEL_RESOURCE_ATTRIBUTES or OTEL_SERVICE_NAME in future, will it get ovewritten by the value of defaultServiceName here?
There was a problem hiding this comment.
The precedence is now options.serviceName > env.SPLUNK_SERVICE_NAME > service.name from env.OTEL_RESOURCE_ATTRIBUTES > defaultServiceName
Description
Adds log injection support for
bunyan,pinoandwinston. This option is disabled by default.Not sure regarding the environment variable name, suggestions welcome.
Also fixed service name not being injected to provider resource.
Ticket ID:
APMI-1628Type of change
How Has This Been Tested?
Checklist: