Skip to content

[internal] Add submit log flag to forward logs to DD - #185

Merged
yoannmoinet merged 13 commits into
masterfrom
yoann/add-submit-log-flag
Jul 9, 2025
Merged

[internal] Add submit log flag to forward logs to DD#185
yoannmoinet merged 13 commits into
masterfrom
yoann/add-submit-log-flag

Conversation

@yoannmoinet

@yoannmoinet yoannmoinet commented Jun 25, 2025

Copy link
Copy Markdown
Member

What and why?

This PR enhances our internal telemetry capabilities by implementing log forwarding to Datadog for better observability of build processes.

The changes enable us to monitor build plugin performance and issues in production environments while ensuring these operations don't impact build performance.

The list of changes can be daunting, but there is a lot of code moving but not a lot of actual changes/additions.

Moves:

  • logger code has been split out.
  • sendLog (to Datadog) has been extracted and moved to core helpers.
  • GlobalContext now use simpler and more contained arguments data and stores.

Changes:

  • Add more mocks to the tests (data, stores, etc...) and factorise others (bundler, logger, etc...).
  • Internal telemetry on our own repository builds.
  • Fix yarn cli create-plugin command.

Additions:

  • async-queue-plugin.
  • { forward: boolean } option to logger.

Key improvements:

  • Internal telemetry: Only enable telemetry for our main build (removed from types generation and sub-builds). This will reduce the amount of metrics we send.
  • Non-blocking operations: New async queue system ensures we can do async actions without delaying the build process, while still waiting for them to finish before quitting the build.
  • Architecture improvements:
    • Better isolation of data and stores in the global context system.
    • Refactored how we log things following the data/stores management changes.
    • Changed how we mock loggers in tests to avoid circular dependencies in some mocking situations.

How?

The implementation is organized around three main themes:

1. Log Forwarding Infrastructure

  • Added submitLog helper in packages/core/src/helpers/log.ts to send logs to Datadog intake API.
  • Refactored logger (packages/factory/src/helpers/logger.ts) to support conditional log forwarding.
    • Simply use log.error('New error happened.', { forward: true }); to forward any log.
  • Refactored context.sendLog following the previous changes.

2. Async Queue Plugin

  • Created new @dd/async-queue-plugin to handle non-blocking operations during the build
    • Using it when sending and forwarding logs to DD.
  • Integrated queue into factory context as context.queue(), allowing plugins to defer work without blocking builds.

3. Architecture Improvements

  • Fixed circular dependencies in test mocks by refactoring timeLoggers mocking approach.
  • Created GlobalStores and GlobalData types to strictly define what the global context is using.
  • GlobalContext is now built from data and stores passed as arguments.

Hopefully this will offer new ways of understanding usages of the plugins while simplifying the architecture of the whole system.

@yoannmoinet
yoannmoinet force-pushed the yoann/add-submit-log-flag branch from cf8ed19 to 27d4c05 Compare June 26, 2025 08:43
},
"devDependencies": {
"typescript": "${pkg.devDependencies.typescript}",
"typescript": "${pkg.devDependencies.typescript}"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a fix for the yarn cli create-plugin command that would produce a faulty package.json breaking any yarn run afterward.


> Send some analytics data to Datadog internally.
> <br/>
> It gives you acces to the `context.sendLog()` function.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we don't give access to context.sendLog anymore, is this a breaking change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, we're still give access to the function, but it's not defined from there anymore.

I forgot to document it from the context now.
Will do.

Comment thread packages/core/src/helpers/log.ts
@yoannmoinet
yoannmoinet marked this pull request as ready for review July 4, 2025 12:31

@sethfowler-datadog sethfowler-datadog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice; this will be a very handy feature to have! LGTM.


if (forward) {
stores.queue.push(
getSendLog(data)({ message: content, context: { plugin: name, status: type } }),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: perhaps call getSendLog() once outside of this closure and capture the result?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong feeling about it, but curious as to why?

if (errors.length > 0) {
log.error(
`Error occurred while processing async queue:\n ${errors.join('\n ')}`,
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's worth setting forward: true here, so that these errors could have a chance to get reported to the server?

It'd admittedly be a bit of a pain, since you have to deal with the fact that the network request will get pushed onto stores.queue, so you'd have to handle that and do a second round of awaiting. (Presumably giving up on forward: true if there are still errors.) So, no strong feelings, but I figured it was worth considering at least.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now it would probably be correct, but the more we go, the more I'd be worried to get all kind of failures unrelated to "us".

Like it could fail from custom plugins for instance.

@yoannmoinet
yoannmoinet merged commit cb65ff9 into master Jul 9, 2025
6 checks passed
@yoannmoinet
yoannmoinet deleted the yoann/add-submit-log-flag branch July 9, 2025 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants