feat: add server timing header injection #70
Conversation
README.md
Outdated
| @@ -68,6 +68,7 @@ You can also instrument your app with code as described [here](#instrument-with- | |||
| | SPLUNK_SERVICE_NAME | serviceName | `unnamed-node-service` | The service name of this Node service. | | |||
| | SPLUNK_ACCESS_TOKEN | acceessToken | | | The optional organization access token for trace submission requests. | | |||
| | SPLUNK_MAX_ATTR_LENGTH | maxAttrLength | 1200 | Maximum length of string attribute value in characters. Longer values are truncated. | | |||
| | SPLUNK_SERVER_TIMING_ENABLED | serverTimingEnabled | false | Enable injection of `Server-Timing` header to HTTP responses. | | |||
There was a problem hiding this comment.
Was decided to default to true
There was a problem hiding this comment.
Java and SFX Py use SPLUNK_CONTEXT_SERVER_TIMING_ENABLED so we should use that in this PR.
I think we should rename it to something more generic. More here: signalfx/gdi-specification#27
There was a problem hiding this comment.
Thanks, fixed! And agree with your proposal 🙂
src/tracing.ts
Outdated
| if (response instanceof ServerResponse) { | ||
| const { traceId, spanId } = span.context(); | ||
| response.setHeader('Access-Control-Expose-Headers', 'Server-Timing'); | ||
| response.setHeader( |
There was a problem hiding this comment.
Will setHeader append or overwrite any existing values? We should be appending in order to not break any services that might use these two headers in a different way.
There was a problem hiding this comment.
With the current http instrumentation, the response hook is called before users have access to the response object (https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-plugin-http/src/http.ts#L316)
However if this gets changed in future versions so it's called somewhere later down the line, the header could be overwritten 🤔
There was a problem hiding this comment.
Added appending of headers: ddccfb7#diff-8e40ce0d3dab91400c9a4d6c3c2577a8222c65dc14f886a2b570b564909bef01R81-R97
Although this will take the first branch 100% of the time, since response is not available for users to modify before the hook is called.
src/tracing.ts
Outdated
| } | ||
| }; | ||
|
|
||
| let config = instrumentation._getConfig() as HttpInstrumentationConfig; |
There was a problem hiding this comment.
can this result in runtime error? can we add guards in case?
There was a problem hiding this comment.
I guess only if the logic of getConfig changes (e.g. adding required arguments), currently it only returns an existing dictionary
There was a problem hiding this comment.
OK. As long as we have a test case that detect such breaking I think we should be good.
There was a problem hiding this comment.
Should be covered by tests in test/servertiming.test.ts
|
Added a check for sampling flags (as discussed on Slack) |
|
Server-Timing is now added by default |
|
Nothing from my side. |
Description
Enable injection of
Server-Timingresponse headers. This also works if an user provides their ownHttpInstrumentationwith an existing response hook, so no additional code changes are needed for them except enabling the feature.Type of change
How Has This Been Tested?
Tested manually with express and koa.
Checklist: