[#439] refactor: decouple UriStats configuration from ConfigBuilder#440
Merged
feelform merged 3 commits intopinpoint-apm:masterfrom Mar 3, 2026
Merged
[#439] refactor: decouple UriStats configuration from ConfigBuilder#440feelform merged 3 commits intopinpoint-apm:masterfrom
feelform merged 3 commits intopinpoint-apm:masterfrom
Conversation
This was
linked to
issues
Feb 26, 2026
…e paths [pinpoint-apm#436] Set pinpoint.metric.uri-template attribute [pinpoint-apm#442] TraceCompletionHooks [pinpoint-apm#443] Dedicated ConfigBuilder for TraceCompletionHooks
…StatsHttpMethodEnabled, isUriStatsUseUserInput, isUriStatsEnabled) in config
There was a problem hiding this comment.
Pull request overview
This PR refactors URI stats configuration so it is no longer owned by ConfigBuilder, and moves URI stats attributes off SpanBuilder into TraceRoot “enrichers” for consumption by the URI stats repository.
Changes:
- Introduces
UriStatsConfigBuilder/UriStatsConfigand updates repository construction to use the new config object. - Switches URI stats capture/storage from
SpanBuilderfields (uriTemplate,httpMethod) toTraceRootenrichers (uriStats.uriTemplate,uriStats.method). - Adds an (unfinished) trace-completion hook mechanism and updates tests/mocks to the new URI stats attribute location.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/support/agent-singleton-mock.js | Builds URI stats repository using UriStatsConfigBuilder instead of global config. |
| test/instrumentation/module/koa.test.js | Updates assertions to read URI stats values from TraceRoot enrichers. |
| test/instrumentation/module/express.test.js | Updates assertions to use TraceRoot enrichers; adds repository null-object assertion when disabled. |
| test/config.test.js | Switches URI stats-related config tests to UriStatsConfigBuilder. |
| lib/metric/uri/koa-uri-stats-trace-completion-hook.js | Adds Koa-specific URI stats completion hook (new file). |
| lib/metric/uri/http-request-uri-stats-trace-completion-hook.js | Adds HTTP-request completion hook for user-input URI template (new file). |
| lib/metric/uri/express-uri-stats-trace-completion-hook.js | Adds Express-specific URI stats completion hook (new file). |
| lib/metric/uri-stats-repository.js | Reads uriTemplate/method from TraceRoot enrichers; builder uses new config shape. |
| lib/metric/uri-stats-config-builder.js | Adds new builder responsible for env/config merge and validation. |
| lib/instrumentation/trace-completion-hook.js | Introduces trace completion hook base class (new file). |
| lib/instrumentation/module/koa/koa-register-interceptor.js | Always records URI template/method via SpanRecorder (now enrichers-based). |
| lib/instrumentation/module/express/express-layer-interceptor.js | Always records URI template/method via SpanRecorder (now enrichers-based). |
| lib/instrumentation/http-shared.js | Records user-input URI template unconditionally (previously gated). |
| lib/context/trace/trace.js | URI stats repository now consumes TraceRoot instead of SpanBuilder. |
| lib/context/trace/trace-root.js | Adds setEnricher / getEnricher support backed by a Map. |
| lib/context/trace/span-recorder.js | Records URI stats data into TraceRoot enrichers instead of SpanBuilder. |
| lib/context/trace-context.js | Adds trace completion hook invocation after trace close. |
| lib/context/span-builder.js | Removes uriTemplate/httpMethod fields from the span model. |
| lib/config-builder.js | Removes URI stats config handling/validator; exports env parsing helpers. |
| lib/config-builder.d.ts | Removes addHandler from typings to match implementation. |
| lib/agent-builder.js | Creates TraceContext earlier; adds API to register trace completion hooks. |
| index.js | Builds and passes uriStatsConfig into makeStatsRepository. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
feelform
force-pushed
the
URL11
branch
2 times, most recently
from
bc78499 to
5933b97
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+310
to
311
| t.equal(conf.capacity, 1000, 'uriStats capacity remains undefined when not set') | ||
| t.equal(conf.getUriStatsCapacity(), 1000, 'getUriStatsCapacity should fall back to 1000 when capacity is undefined but uriStats exists') |
There was a problem hiding this comment.
The assertion expects conf.capacity to be 1000, but the assertion message says it "remains undefined". Please update the message (or expectation) so the test output matches the intended behavior (capacity defaults to 1000 when uriStats is enabled via httpMethod).
Suggested change
| t.equal(conf.capacity, 1000, 'uriStats capacity remains undefined when not set') | |
| t.equal(conf.getUriStatsCapacity(), 1000, 'getUriStatsCapacity should fall back to 1000 when capacity is undefined but uriStats exists') | |
| t.equal(conf.capacity, 1000, 'uriStats capacity should default to 1000 when not set') | |
| t.equal(conf.getUriStatsCapacity(), 1000, 'getUriStatsCapacity should be 1000 when capacity uses the default and uriStats exists') |
Comment on lines
49
to
+57
| try { | ||
| trace.close() | ||
| for (const hook of this.traceCompletionEnrichers) { | ||
| try { | ||
| hook.onComplete(trace) | ||
| } catch (e) { | ||
| log.error('Hook failed', e) | ||
| } | ||
| } |
There was a problem hiding this comment.
traceCompletionEnrichers are invoked after trace.close(), but trace.close() already stores the span/URI stats. Any enricher that tries to mutate the trace/span at completion time will have no effect on what gets sent/stored. Consider running completion enrichers before trace.close() (or splitting into pre-close vs post-close hooks / renaming to avoid implying mutation is possible).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.