-
Notifications
You must be signed in to change notification settings - Fork 810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(sdk-trace-base): log resource attributes in ConsoleSpanExporter #4605
feat(sdk-trace-base): log resource attributes in ConsoleSpanExporter #4605
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4605 +/- ##
=======================================
Coverage 92.83% 92.83%
=======================================
Files 329 329
Lines 9528 9528
Branches 2053 2053
=======================================
Hits 8845 8845
Misses 683 683
|
Not sure if adding immutable |
Yes it's not optimal, I think that was the original reason why this way not added.
We rejected logging once at some point in the past as we'd have to build a map to organize the data as an equivalent to I'd also like to avoid increasing the API surface for this if at all possible. Opting-in for this kind of situation would require users to pass an extra option to I think that the extra verbosity is the best compromise until we can change the exporter-interface to require pre-organized data in SDK 2.0. |
config could happen via env similar to other exporters which, e.g. But well, that likely needs spec work first so out of scope here. |
I was playing with this a little bit... but then I noticed that given Perhaps that's expected for debugging -- don't want possible issues/confusion from batching to get in the way? Experiment A
--- a/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts
+++ b/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts
@@ -168,7 +168,7 @@ export class TracerProviderWithEnvExporters extends NodeTracerProvider {
exporters: SpanExporter[]
): (BatchSpanProcessor | SimpleSpanProcessor)[] {
return exporters.map(exporter => {
- if (exporter instanceof ConsoleSpanExporter) {
+ if (false && exporter instanceof ConsoleSpanExporter) {
return new SimpleSpanProcessor(exporter);
} else {
return new BatchSpanProcessor(exporter);
diff --git a/packages/opentelemetry-sdk-trace-base/src/export/ConsoleSpanExporter.ts b/packages/opentelemetry-sdk-trace-base/src/export/ConsoleSpanExporter.ts
index 596b3cef..afe0c51a 100644
--- a/packages/opentelemetry-sdk-trace-base/src/export/ConsoleSpanExporter.ts
+++ b/packages/opentelemetry-sdk-trace-base/src/export/ConsoleSpanExporter.ts
@@ -86,8 +86,26 @@ export class ConsoleSpanExporter implements SpanExporter {
spans: ReadableSpan[],
done?: (result: ExportResult) => void
): void {
+ const resourceSpanss = [];
for (const span of spans) {
- console.dir(this._exportInfo(span), { depth: 3 });
+ const lastResourceSpans = resourceSpanss[resourceSpanss.length - 1];
+ if (!lastResourceSpans || span.resource !== lastResourceSpans.resource) {
+ resourceSpanss.push({
+ resource: span.resource,
+ spans: [span],
+ });
+ } else {
+ lastResourceSpans.spans.push(span);
+ }
+ }
+ for (const resourceSpans of resourceSpanss) {
+ const repr = {
+ resource: {
+ attributes: resourceSpans.resource.attributes,
+ },
+ spans: resourceSpans.spans.map(this._exportInfo),
+ };
+ console.dir(repr, { depth: 3 });
}
if (done) {
return done({ code: ExportResultCode.SUCCESS }); Running that with an example script that generates a number of spans manually looks like this:
Thoughts? |
Experiment B
_sendSpans(spans, done) {
for (const span of spans) {
const repr = this._exportInfo(span);
if (span.resource === this._lastResource) {
repr.resource = Symbol.for('ibid');
} else {
repr.resource = { attributes: span.resource.attributes };
this._lastResource = span.resource;
}
console.dir(repr, { depth: 3 });
}
// ... It looks like this:
Thoughts? |
I wonder why exporter gets more spans in B if In my opinion If If someone has a need for an optimized console exporter because of a special setup it's not that hard to create a custom variant. By the way, does anyone know what other techs do with resource in console exporter? |
That's also my understanding. @trentm Is is possible that this was using the BatchSpanProcessor? 🤔
Yes,
Agree.
Haha, I wonder if anyone will ever again think about this as hard as we do now. 😄 |
Pretty sure it was with SimpleSpanProcessor. That |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this -- though I kinda like the ibid
thing. ;)
Let's go with the change from this PR first. We can then iterate on it if users find that logging the resource every time is too verbose for debugging purposes. |
…pen-telemetry#4605) * feat(sdk-trace-base): log resoruce attributes in ConsoleSpanExporter * fixup! feat(sdk-trace-base): log resoruce attributes in ConsoleSpanExporter
…pen-telemetry#4605) * feat(sdk-trace-base): log resoruce attributes in ConsoleSpanExporter * fixup! feat(sdk-trace-base): log resoruce attributes in ConsoleSpanExporter
Which problem is this PR solving?
It's currently impossible to troubleshoot resource setup without explicitly logging the resource before setting up the SDK.
This PR adds the span's resource to the output that's logged to the console to aid troubleshooting.
Type of change