feat(sdk-logs,sdk-trace-base,sdk-metrics): add configuration for console.dir depth in ConsoleSpanExporter, ConsoleMetricExporter, and ConsoleLogRecordExporter

[btomaj]

Which problem is this PR solving?

This PR makes console.dir depth configurable in ConsoleSpanExporter, ConsoleMetricExporter, and ConsoleLogRecordExporter to resolve problem using with Cloudflare (excessive depth), and Semantic Conventions for Generative AI (insufficient depth).

Fixes #5800

Short description of the changes

1. An optional options parameter is added to the constructors of ConsoleSpanExporter and ConsoleLogRecordExporter.
2. An optional depth property is added to the options parameter of the constructors of ConsoleSpanExporter, ConsoleMetricExporter, and ConsoleLogRecordExporter that controls the depth setting of console.dir().

Type of change

• New feature (non-breaking change which adds functionality)

NB: ConsoleSpanExporter, ConsoleMetricExporter, and ConsoleLogRecordExporter are not well documented, only used in examples, so there was no existing documentation to update.

How Has This Been Tested?

• packages/opentelemetry-sdk-trace-base/test/common/export/ConsoleSpanExporter.test.ts
• packages/sdk-metrics/test/export/ConsoleMetricExporter.test.ts
• experimental/packages/sdk-logs/test/common/export/ConsoleLogRecordExporter.test.ts

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[cjihrig]

It's worth noting a few things:

• There are other console.dir() flags besides depth. If this were to be added, it would feel incomplete to only support depth.
• Avoid using console.dir in ConsoleSpanExporter for Cloudflare Worker compatibility #5800 and the related workerd issue seem to imply that console.dir() doesn't work at all in Workers, independent of depth. If that's the case, this will not fix that issue.

[btomaj]

• There are other console.dir() flags besides depth. If this were to be added, it would feel incomplete to only support depth.

I agree, and I also don't think depth is a very good name. I would have changed the default to null instead of adding it as an option, but I didn't know why it had been set to 3 (instead of the default of 2 when the option is undefined), so it seemed safest to me to declare it as a configurable option. Why has the default been set to 3, and is setting the default to null an option?

• Avoid using console.dir in ConsoleSpanExporter for Cloudflare Worker compatibility #5800 and the related workerd issue seem to imply that console.dir() doesn't work at all in Workers, independent of depth. If that's the case, this will not fix that issue.

On a second reading, I see that you are right. I misunderstood the conversation. Would something like this be a better change?

import { inspect } from 'node:util';

console.log(inspect(obj, { depth: null, colors: true }));

[cjihrig]

I agree, and I also don't think depth is a very good name.

depth is a fine name, and matches the actual parameter name used by console.dir(). I was just pointing out that there are other parameters that probably make sense to include if this were to land.

Would something like this be a better change?

In Node, that general approach would likely be fine because that's how console.dir() is actually implemented in Node, so that should solve the Cloudflare use case (although this really should be fixed on their side). I'm unsure about any implications for the browser though. You'll probably need an OTel maintainer to weigh in on that. You also still have the issue of whether or not to make the inspect() options configurable.

[pichlermarc]

In general, the idea of that exporter is to provide enough info for diagnostics use, if it's not doing that then that's a problem - it should not need configuration to work properly. Since a significant chunk of users use this exporter for troubleshooting via NodeSDK, not having a spec-defined environment variable for configuring the console exporter means that adding config options would be of them - it needs to work OOTB.

To solve both the problems of depth for GenAI SemConv and the compatibliity with CloudFlare workers, I'd suggest the following approach:

• implement custom functionality to stringify the LogRecord into a readable format including any arbitrary-depth attributes (may use whatever is appropriate and widely available - JSON.stringify() maybe; we may not use inspect() as it's a Node.js API that's not available everywhere)
• log that using plain console.log() instead
• make the same changes to metrics/trace console exporters

We will loose the fancy formatting we get with console.dir() but that's an okay trade-off IMO.

[btomaj]

@pichlermarc thank you for providing that context. What do you think about a solution that relies on console.dir with null depth where available, and that falls back to JSON.stringify and console.log?

[btomaj]

@pichlermarc, I have updated logging to use, in order of preference:

1. console.dir with { depth : null }
2. console.log with JSON.stringify(..., null, 2)
3. console.log with String(...)

This should resolve everyone's issues.

[pichlermarc]

Looks like we're trying to solve two distinct issues here with completely different requirements which makes this PR stall and take longer than needed:

1. output is insufficient for AI semconv (no issue created yet, AFAIK)
2. cloudflare workers does not implement the WhatWG console spec correctly (cloudflare/workerd#2247, #5800)

Consider solving one at a time to limit discussions:

• just extending the console.dir() to use depth: null can be one PR.
• Solving the Cloudflare Workers issue can be another PR

[pichlermarc]

I think the issue in Cloudflare workers is that the function exists but it just does not do anything. So if we want to do feature-detection we have to do it another way - otherwise the issue won't be solved here.

ref #5800
ref cloudflare/workerd#2247

[btomaj]

@pichlermarc thank you for reviewing. You're right that I'm trying to solve two different problems. Solving #5800 should also solve the issue with Semantic Conventions for Generative AI, so would you prefer that I remove console.dir, and rely on JSON.stringify and fall back to String in line with your first comment?

[codecov]

Codecov Report

❌ Patch coverage is 90.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.14%. Comparing base (13c0790) to head (0a034af).
⚠️ Report is 29 commits behind head on main.

Files with missing lines	Patch %	Lines
...es/sdk-logs/src/export/ConsoleLogRecordExporter.ts	90.00%	1 Missing ⚠️
...y-sdk-trace-base/src/export/ConsoleSpanExporter.ts	90.00%	1 Missing ⚠️
...es/sdk-metrics/src/export/ConsoleMetricExporter.ts	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6056      +/-   ##
==========================================
- Coverage   95.16%   95.14%   -0.02%     
==========================================
  Files         316      316              
  Lines        9198     9247      +49     
  Branches     2057     2086      +29     
==========================================
+ Hits         8753     8798      +45     
- Misses        445      449       +4     

Files with missing lines	Coverage Δ
...es/sdk-logs/src/export/ConsoleLogRecordExporter.ts	85.71% <90.00%> (+2.38%)	⬆️
...y-sdk-trace-base/src/export/ConsoleSpanExporter.ts	84.00% <90.00%> (+2.75%)	⬆️
...es/sdk-metrics/src/export/ConsoleMetricExporter.ts	90.90% <90.00%> (-0.76%)	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pichlermarc]

@pichlermarc thank you for reviewing. You're right that I'm trying to solve two different problems. Solving #5800 should also solve the issue with Semantic Conventions for Generative AI, so would you prefer that I remove console.dir, and rely on JSON.stringify and fall back to String in line with your first comment?

yes, I think that works :)

[btomaj]

@pichlermarc, by default JSON.stringify() will drop undefined keys, but the tests currently expect all keys to be present. I can:

1. update the test to ensure no value is undefined,
2. update the test to only check defined values, or
3. add (_key, value) => (value === undefined ? null : value) as the replacer argument for JSON.stringify() to ensure undefined values are displayed as null.

What is the preferred approach?

[pichlermarc]

@pichlermarc, by default JSON.stringify() will drop undefined keys, but the tests currently expect all keys to be present. I can:

1. update the test to ensure no value is undefined,
2. update the test to only check defined values, or
3. add (_key, value) => (value === undefined ? null : value) as the replacer argument for JSON.stringify() to ensure undefined values are displayed as null.

What is the preferred approach?

I prefer Option 3 - in OTLP an empty attribute value should be encoded as a KeyValue that includes the key as well as a zero-length value. An empty string/empty object could also work to convey that, but null works best IMO.

[btomaj]

@pichlermarc thanks for all your help with this. I have updated the PR, and I believe that the commit should now resolve #5800. Please let me know if you have further feedback on the change. In particular, please review the comment in ConsoleMetricExporter.ts

[btomaj]

@pichlermarc just following up on this PR. Are there any other changes required?