feat: w3c field - #201
feat: w3c field#201maxday wants to merge 5 commits into
Conversation
Exposes W3C trace-context fields (traceparent, tracestate, baggage) carried on clientContext.w3c via a new context.w3c() helper. Fields are allowlisted; the source clientContext.w3c is removed after construction so callers only see the helper.
| --build-arg BASE_IMAGE=public.ecr.aws/lambda/nodejs:24 | ||
|
|
||
| - name: Run dockerized suites | ||
| uses: aws/containerized-test-runner-for-aws-lambda@76eacfb110903739d9c5f7fcdaafede6324a1473 # maxday/client-context |
There was a problem hiding this comment.
will change to change when aws/containerized-test-runner-for-aws-lambda#32 will be reviewed and merged
| }, | ||
| w3c: function (): Record<string, string> { | ||
| return { ...w3cFields }; | ||
| }, |
There was a problem hiding this comment.
What was the intention behind returning a copy from a function here? I can think of two:
-
- Keeping w3c out of logs. Functions are skipped by JSON.stringify(context), so that works, but it's an implicit side effect. If this is the goal, I'd make it explicit with a non-enumerable property, which hides it from JSON.stringify and console.log while keeping it readable:
Object.defineProperty(context, "w3c", {
value: w3cFields,
enumerable: false,
writable: false,
});-
- Preventing handlers from mutating the fields. { ...w3cFields } creates a new object on every call, while identity and clientContext on the same object stay mutable, so the protection is inconsistent. I think the direction should be making the whole context immutable rather than protectingthe full context is a breaking change (handlers and middleware write to it today), sothat's probably a separate discussion. For this PR I'd return a frozen object instead of a copy, so w3c is consistent with where we want to go:
// extractAndStripW3c
return Object.freeze(fields);
// With the object frozen, the function isn't needed anymore either; a plain property is srantees:
readonly w3c: Readonly<Record<string, string>>;(non-enumerable as above if we want it out of logs). Since this changes context.w3c() to settle before this ships.
| private static extractAndStripW3c( | ||
| clientContext: Record<string, unknown> | undefined, | ||
| ): Record<string, string> { | ||
| if (!clientContext || typeof clientContext !== "object") { |
There was a problem hiding this comment.
nit: Maybe in another PR but this check should be guaranteed by parseJsonHeader.
| if (!clientContext || typeof clientContext !== "object") { | ||
| return {}; | ||
| } | ||
| if (!("w3c" in clientContext)) { |
There was a problem hiding this comment.
nit: merge the condition with above.
| const source = rawW3c as Record<string, unknown>; | ||
| const fields: Record<string, string> = {}; | ||
| for (const key of W3C_ALLOWED_FIELDS) { | ||
| const value = source[key]; | ||
| if (typeof value === "string") { | ||
| fields[key] = value; | ||
| } | ||
| } | ||
| return fields; |
There was a problem hiding this comment.
The W3C standard tells you that every implementor should validate those fields before sharing it.
This include existent validation. For example if TraceParent is not present you should not TraceState.
They also should be validated through a RegEXP.
I did it in my solution. I can show you how it is done.
|
|
||
| # Copy bare config | ||
| COPY package.json tsconfig.json eslint.config.js vitest.config.js vitest.setup.ts /app/ | ||
| COPY package.json package-lock.json tsconfig.json eslint.config.js vitest.config.js vitest.setup.ts /app/ |
There was a problem hiding this comment.
Very nice find! Very important 🚀
| WORKDIR /app | ||
|
|
||
| RUN npm install --ignore-scripts | ||
| RUN npm ci --ignore-scripts |
There was a problem hiding this comment.
note: we should move to pnpm.
| getRemainingTimeInMillis: function () { | ||
| return deadline - Date.now(); | ||
| }, | ||
| w3c: function (): Record<string, string> { |
There was a problem hiding this comment.
Another important thing is that. traceparent and tracestate belong to the same group and they are dependant on each other. While baggage is completely indipendent. We should not group them under the the same object. They can exist separately.
There was a problem hiding this comment.
Probably the best shape would be to typer both. I have examples also for that that i can show.
| "baggage", | ||
| ] as const; | ||
|
|
||
| export type W3CFieldName = (typeof W3C_ALLOWED_FIELDS)[number]; |
| @@ -0,0 +1,149 @@ | |||
| { | |||
There was a problem hiding this comment.
nit: lots of cases here. Are we sure to create such a long test for an integration test that is already covered by unit test?
This branch has not been deployed
2 participants
Adds
context.w3c()which exposes allowlisted W3C trace-context fields (traceparent,tracestate,baggage) carried onclientContext.w3c, and stripsw3cfromclientContextso callers can only read trace fields through the helper. Covered by unit tests plus a new dockerized end-to-end workflow againstpublic.ecr.aws/lambda/nodejs:24.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.