You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
PR Review — Score: 4.4 / 5
This is a focused, well-tested fix for real observability gaps: main-process RUM events now carry the expected ddtags, and intake uploads include the standard SDK query parameter and headers. The changes are localized, backward-compatible, and covered by new unit tests plus e2e assertions on event bodies. I would approve.
Why 4.4: Correct attribution logic (buildDdtags), proper proxy ddforward encoding for ddsource, fresh per-upload DD-REQUEST-ID, and solid unit/e2e coverage for the event-side changes. Unit and lint CI are already green on the head commit.
Why not 5: E2e still does not assert intake request metadata (headers / ddsource) even though the fake intake captures it, and transport conventions are not documented. A minor edge case around unescaped commas in tag values remains unhandled.
Findings
[Minor] E2e intake metadata gap — Scenario validates event ddtags but not proxy request headers or ddsource in ddforward.
[Minor] Missing transport docs — New intake headers/query param are undocumented for maintainers.
[Nit] ddtags value escaping — buildDdtags joins raw config strings; commas in values would corrupt the tag list.
Architectural flow
sequenceDiagram
participant Main as Main RUM pipeline
participant Ctx as commonContext.buildDdtags
participant Batch as BatchProducer
participant Consumer as StandardBatchConsumer
participant Intake as Datadog intake (or proxy)
Main->>Ctx: registerCommonContext on init
Ctx-->>Main: ddtags with sdk_version, service, env, version
Main->>Batch: serialized RUM event lines
Batch->>Consumer: upload() per batch file
Consumer->>Consumer: computeIntakeUrlForTrack (?ddsource=electron)
Consumer->>Consumer: buildRequest (+ DD-EVP-ORIGIN, DD-REQUEST-ID)
Consumer->>Intake: POST JSON array
Loading
Before: Main-process RUM ddtags only contained sdk_version; intake POSTs used a bare /api/v2/{track} URL and only DD-API-KEY / Content-Type / User-Agent headers.
After:buildDdtags mirrors browser-SDK tag conventions on main-process events, and every standard batch upload identifies itself as the Electron SDK via ddsource=electron, DD-EVP-ORIGIN, DD-EVP-ORIGIN-VERSION, and a per-request DD-REQUEST-ID.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
PR Review — Score: 4.5 / 5
This is a focused, well-tested fix for missing main-process RUM attribution and intake metadata. The changes land in the right layers (buildDdtags for event assembly, computeIntakeUrlForTrack for ddsource, StandardBatchConsumer for EVP headers), unit tests cover the new behavior including comma sanitization and per-request UUIDs, and the e2e startup view scenario validates the end-to-end path. I would approve.
Why 4.5: Clear motivation, additive/backward-compatible changes, solid unit + e2e coverage, and proactive ddtags sanitization added after the initial commit.
Why not 5: Tag sanitization is narrower than the failure mode it guards against, documentation is still unchecked on the PR checklist, and e2e coverage stops at headers without asserting the ddsource query param (unit tests cover it).
Findings
[Minor] Narrow tag sanitization — sanitizeTagValue only replaces commas; colons in config values can still corrupt ddtags parsing.
[Nit] Documentation gap — Related docs were not updated despite new intake headers and ddtags fields.
Architectural flow
sequenceDiagram
participant Init as SDK init
participant Ctx as registerCommonContext
participant Tags as buildDdtags
participant Batch as StandardBatchConsumer
participant URL as computeIntakeUrlForTrack
participant Intake as Datadog intake
Init->>Ctx: configuration (env, service, version)
Ctx->>Tags: build ddtags for MAIN RUM events
Tags-->>Ctx: sdk_version, service, env, version tags
Ctx-->>Batch: RUM events written to .log batches
Batch->>URL: resolve intake URL per track
URL-->>Batch: /api/v2/{track}?ddsource=electron
Batch->>Intake: POST with DD-EVP-ORIGIN, DD-EVP-ORIGIN-VERSION, DD-REQUEST-ID
Loading
Before: Main-process RUM ddtags carried only sdk_version; intake URLs omitted ddsource, and upload requests lacked the standard EVP origin/request-id headers.
After: Configuration env/service/version flow into main-process ddtags (with comma sanitization), every intake URL includes ddsource=electron (including inside ddforward for proxies), and each batch upload carries fresh EVP metadata headers.
- align tag sanitization with browser-sdk: warn via displayWarn for forbidden characters, only replace commas locally
- expose ddforward on ReceivedEvent and assert ddsource=electron in e2e startup view test
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
PR Review — Score: 4.7 / 5
This PR cleanly closes real observability gaps: main-process RUM events now carry env/service/version in ddtags, and intake uploads include ddsource=electron plus the standard EVP headers and per-request DD-REQUEST-ID. The layering is right (buildDdtags in assembly, URL construction in computeIntakeUrlForTrack, headers in StandardBatchConsumer), coverage is strong (new unit specs, expanded consumer tests, e2e assertions on event bodies, headers, and ddforward), and the follow-up commit sensibly addressed prior feedback on comma sanitization and end-to-end intake contract checks. I would approve.
Why 4.7: Correct fix at the right boundaries, backward-compatible additive metadata, thorough tests including proxy ddforward encoding, and pragmatic tag handling (comma replacement + warn) that prevents the worst ddtags corruption without over-sanitizing allowed characters like :.
Why not 5: Tag normalization still diverges slightly from browser-sdk (warn-only for most invalid characters, no client-side lowercasing/truncation), and customer-facing docs were not updated despite the PR checklist item remaining unchecked.
Findings
[Minor] Partial browser-sdk tag parity — Forbidden-character handling warns but does not normalize values client-side the way browser-sdk does (lowercase, replace, truncate).
[Nit] Documentation gap — docs/DEVELOPMENT.md still only mentions sdk_version in RUM ddtags; the new env/service/version tags are undocumented.
Architectural flow
sequenceDiagram
participant Main as Main RUM assembly
participant CC as commonContext.buildDdtags
participant BM as BatchManager
participant BC as StandardBatchConsumer
participant URL as computeIntakeUrlForTrack
participant DD as Datadog intake
Main->>CC: registerCommonContext (MAIN source)
CC-->>Main: ddtags with sdk_version, service, env, version
Main->>BM: post enriched RUM event
BM->>BC: upload batch
BC->>URL: intakeUrl (includes ?ddsource=electron)
BC->>DD: POST with DD-EVP-ORIGIN, DD-EVP-ORIGIN-VERSION, DD-REQUEST-ID
Loading
Before: Main-process RUM events carried only sdk_version in ddtags, and intake POSTs lacked ddsource and standard EVP request headers.
After: Event assembly enriches main-process ddtags from init config, while the transport layer adds ddsource=electron to the intake URL (including inside proxy ddforward) and attaches EVP origin/version plus a fresh request ID on every batch upload.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
PR Review — Score: 4.8 / 5
This PR is ready to merge. It closes real observability gaps with well-scoped changes in the right layers: buildDdtags for main-process RUM attribution, computeIntakeUrlForTrack for ddsource, and StandardBatchConsumer for standard EVP headers. Follow-up commits since the last review addressed prior feedback (comma sanitization, ddforward e2e assertions, 200-character tag warnings), and unit plus e2e coverage is thorough.
Why 4.8: Correct boundary placement, strong test coverage (unit + e2e intake contract), sensible browser-sdk-aligned tag handling, and responsive iteration on review feedback.
Why not 5: Documentation was not updated despite the expanded intake/event metadata surface, and tag normalization intentionally stops at warn-plus-comma-replace (browser-sdk parity) rather than full client-side normalization.
Findings
[Minor] Missing documentation — PR checklist leaves docs unchecked; docs/DEVELOPMENT.md still only mentions __SDK_VERSION__ in ddtags and does not cover intake ddsource or EVP headers.
[Nit] Residual tag normalization gap — Client-side handling matches browser-sdk warn-only semantics; uppercase or oversized values are warned but not lowercased/truncated locally (backend is expected to sanitize).
Architectural flow
sequenceDiagram
participant Init as SDK init
participant Ctx as registerCommonContext
participant Batch as StandardBatchConsumer
participant URL as computeIntakeUrlForTrack
participant Intake as Datadog intake
Init->>Ctx: configuration (env, service, version)
Ctx->>Ctx: buildDdtags() → sdk_version, service, env, version
Note over Ctx: MAIN RUM events carry enriched ddtags + top-level service/version
Batch->>URL: resolve intake URL per track
URL-->>Batch: /api/v2/{track}?ddsource=electron
Note over URL: ddsource embedded in ddforward when proxy is set
Batch->>Intake: POST JSON array
Note over Batch,Intake: DD-API-KEY, DD-EVP-ORIGIN, DD-EVP-ORIGIN-VERSION, DD-REQUEST-ID
Loading
Before: Main-process RUM ddtags carried only sdk_version; intake POSTs omitted ddsource and standard EVP request headers.
After: Event assembly enriches ddtags with configured env/service/version (comma-safe), while transport adds ddsource=electron to the intake URL (including proxied ddforward) and attaches EVP origin/version/request-id headers on every upload.
[Minor] Missing documentation — PR checklist leaves docs unchecked; docs/DEVELOPMENT.md still only mentions SDK_VERSION in ddtags and does not cover intake ddsource or EVP headers.
Existing doc is on build time constant, no need to document anything else
[Nit] Residual tag normalization gap — Client-side handling matches browser-sdk warn-only semantics; uppercase or oversized values are warned but not lowercased/truncated locally (backend is expected to sanitize).
The reason will be displayed to describe this comment to others. Learn more.
PR Review — Score: 4.9 / 5
This PR is ready to merge. It fixes real observability gaps with well-scoped, backward-compatible changes: main-process RUM events now carry env/service/version in ddtags, and intake uploads include ddsource=electron plus standard EVP headers with a fresh DD-REQUEST-ID per batch. Follow-up commits since the first review addressed every prior concern (comma sanitization, 200-character tag warnings, e2e intake contract assertions on headers and ddforward), and the author confirmed intentional choices on browser-sdk tag parity and ddsource for all tracks. All CI checks pass on the head commit.
Why 4.9: Correct layering (buildDdtags in assembly, URL construction in computeIntakeUrlForTrack, headers in StandardBatchConsumer), thorough unit and e2e coverage, responsive iteration on review feedback, and author rationale for remaining design choices.
Why not 5: Tag handling deliberately matches browser-sdk warn-only semantics (no client-side lowercasing/truncation), and runtime intake metadata is intentionally out of scope for docs/DEVELOPMENT.md (build-time constants only). Both are reasonable product decisions, not defects.
Architectural flow
sequenceDiagram
participant Init as SDK init
participant Ctx as registerCommonContext
participant Batch as StandardBatchConsumer
participant URL as computeIntakeUrlForTrack
participant Intake as Datadog intake
Init->>Ctx: configuration (env, service, version)
Ctx->>Ctx: buildDdtags() → sdk_version, service, env, version
Note over Ctx: MAIN RUM events carry enriched ddtags + top-level service/version
Batch->>URL: resolve intake URL per track
URL-->>Batch: /api/v2/{track}?ddsource=electron
Note over URL: ddsource embedded in ddforward when proxy is set
Batch->>Intake: POST JSON array
Note over Batch,Intake: DD-API-KEY, DD-EVP-ORIGIN, DD-EVP-ORIGIN-VERSION, DD-REQUEST-ID
Loading
Before: Main-process RUM ddtags carried only sdk_version; intake POSTs omitted ddsource and standard EVP request headers.
After: Event assembly enriches ddtags with configured env/service/version (comma-safe, browser-sdk-aligned warnings), while transport adds ddsource=electron to the intake URL (including proxied ddforward) and attaches EVP origin/version/request-id headers on every upload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
bcaudan
force-pushed
the
bcaudan/missing-env
branch
from
Motivation
Several fields were missing from main-process RUM events and intake requests, causing incomplete event attribution and non-standard request metadata:
env,service, andversionwere absent fromddtagson main-process RUM eventsddsource,DD-EVP-ORIGIN,DD-EVP-ORIGIN-VERSION, andDD-REQUEST-IDChanges
buildDdtagshelper incommonContext.tsto includesdk_version,service,env, andversioninddtagsfor main-process RUM eventsddsource=electronas a URL query parameter incomputeIntakeUrlForTrack(insideddforwardwhen using a proxy)DD-EVP-ORIGIN: electron,DD-EVP-ORIGIN-VERSION, andDD-REQUEST-ID(fresh UUID per request) headers to intake requestsTest instructions
Configured env parameter should now be attached to main-process RUM events
Checklist