opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

feat(sdk-logs): added colors option to ConsoleLogRecordExporter

Open 10xLaCroixDrinker opened this issue 4 months ago • 8 comments

Which problem is this PR solving?

I'm running this exporter in a worker thread (using a pino logger), and since in that context process.stdout is not a TTY, the logs are not colorized. I'd like the option to force the colorization.

Short description of the changes

Added an optional config for the ConsoleLogRecordExporter that has a single option colors which can override the automatic color support detection to turn colorization on or off.

Type of change

Please delete options that are not relevant.

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [x] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added new unit tests that cover the change
  • Installed into another project and validated manually

Checklist:

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

10xLaCroixDrinker avatar Mar 04 '24 15:03 10xLaCroixDrinker

👋 @open-telemetry/javascript-approvers

10xLaCroixDrinker avatar Mar 11 '24 18:03 10xLaCroixDrinker

Codecov Report

Merging #4524 (885cc15) into main (f6a075b) will increase coverage by 0.00%. The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4524   +/-   ##
=======================================
  Coverage   92.84%   92.85%           
=======================================
  Files         328      328           
  Lines        9494     9498    +4     
  Branches     2040     2042    +2     
=======================================
+ Hits         8815     8819    +4     
  Misses        679      679           
Files Coverage Δ
...es/sdk-logs/src/export/ConsoleLogRecordExporter.ts 90.00% <100.00%> (+2.50%) :arrow_up:

codecov[bot] avatar Mar 11 '24 18:03 codecov[bot]

I only use the ConsoleLogRecordExporter in NODE_ENV=development

10xLaCroixDrinker avatar Mar 12 '24 12:03 10xLaCroixDrinker

I'm using pino for logging with pino-opentelemetry-transport. Since its running in a worker thread there is never colorization. Currently, I'm conditionally setting process.env.FORCE_COLORS="1" when I create the exporter to provide this. What I'd like to do is pass my options as { colors: process.stdout.isTTY } from the main thread.

10xLaCroixDrinker avatar Mar 12 '24 22:03 10xLaCroixDrinker

@pichlermarc what do you think?

10xLaCroixDrinker avatar Mar 18 '24 16:03 10xLaCroixDrinker

@10xLaCroixDrinker would it help to have colorized output be the default behavior (also for metric and trace console exporters)? :thinking:

I'm thinking something like: if process is defined (may not be defined in the browser) and output is TTY, then automatically set the option. This way we would not add to the API surface of the ConsoleExporter :slightly_smiling_face:

pichlermarc avatar Mar 19 '24 08:03 pichlermarc

@pichlermarc that is already the default behavior when process.stdout is a TTY. I'm running into this because the exporter is in a worker thread where it is not a TTY. When I say that I'd set the config to { colors: process.stdout.isTTY }, that would just be when I'm setting the config in the main thread. When the exporter is created in the worker thread, it will just receive { colors: true }

10xLaCroixDrinker avatar Mar 20 '24 15:03 10xLaCroixDrinker

Sorry for leaving you waiting for so long.

@pichlermarc that is already the default behavior when process.stdout is a TTY. I'm running into this because the exporter is in a worker thread where it is not a TTY. When I say that I'd set the config to { colors: process.stdout.isTTY }, that would just be when I'm setting the config in the main thread. When the exporter is created in the worker thread, it will just receive { colors: true }

I see.

I'd like to avoid having to increase the API surface for the console exporters. A reason why we have not added any features/config options to any of the other console exporters in the past was that the exporter interface is reasonably easy to implement on one's own. We therefore usually recommend users to implement a custom exporter for use-cases like this.

While I see that the feature can be useful we'd still want to apply the same principle for this exporter.

pichlermarc avatar Apr 05 '24 14:04 pichlermarc