Repository navigation
Conversation
📋 Deprecation NoticeThis PR has been labeled as a deprecation. Please ensure you provide the following information: 📝 Deprecation Details RequiredPlease add details to help users understand:
📋 Checklist
Your deprecation notes will be included in the release notes to help users prepare for future changes. This comment was automatically generated because the |
There was a problem hiding this comment.
Pull request overview
Adds pattern-based HTTP header capture selectors to Restlet and Servlet library instrumentation.
Changes:
- Adds
IncludeExcluderequest/response header APIs while deprecating legacy setters. - Implements request and response header-name enumeration.
- Adds wildcard, exclusion, and case-insensitive capture tests.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents the API migration. |
servlet-common/.../ServletAccessor.java |
Adds response header-name access. |
servlet-common/.../ServletHttpAttributesGetter.java |
Exposes servlet header names. |
servlet-3.0/.../ServletTelemetryBuilder.java |
Adds selector-based APIs. |
servlet-3.0/.../Servlet3Accessor.java |
Enumerates response headers. |
servlet-3.0/.../ServletHeaderCaptureTest.java |
Tests selector capture. |
servlet-3.0/.../ServletTestUtil.java |
Uses the new APIs. |
servlet-5.0/.../ServletTelemetryBuilder.java |
Adds selector-based APIs. |
servlet-5.0/.../Servlet5Accessor.java |
Enumerates response headers. |
servlet-5.0/.../ServletHeaderCaptureTest.java |
Tests selector capture. |
servlet-5.0/.../tomcat/ServletTestUtil.java |
Updates deprecation context. |
restlet-1.1/.../RestletTelemetryBuilder.java |
Adds selector-based APIs. |
restlet-1.1/.../RestletHttpAttributesGetter.java |
Enumerates header names. |
restlet-1.1/.../RestletHttpAttributesGetterTest.java |
Tests selector behavior. |
restlet-1.1/.../RestletServerTest.java |
Uses the new APIs. |
restlet-1.1/.../spring/AbstractSpringServerLibraryTest.java |
Suppresses intentional deprecated usage. |
restlet-2.0/.../RestletTelemetryBuilder.java |
Adds selector-based APIs. |
restlet-2.0/.../RestletHttpAttributesGetter.java |
Enumerates header names. |
restlet-2.0/.../RestletHttpAttributesGetterTest.java |
Tests selector behavior. |
restlet-2.0/.../RestletServerTest.java |
Uses the new APIs. |
restlet-2.0/.../spring/AbstractSpringServerLibraryTest.java |
Suppresses intentional deprecated usage. |
Suppressed comments (4)
instrumentation/restlet/restlet-1.1/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_1/RestletTelemetryBuilder.java:103
- [General] The removal timeline conflicts with #19444, which keeps legacy setters through 2.x and removes them in 3.0; the delegated extractor API also documents removal in 3.0. Saying “next minor release” incorrectly tells users this API may disappear during 2.x. Please align the Javadoc and annotation comment with the 3.0 timeline.
* @deprecated Use {@link #setResponseHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
*/
@Deprecated // may be removed in the next minor release
instrumentation/restlet/restlet-2.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v2_0/RestletTelemetryBuilder.java:103
- [General] The removal timeline conflicts with #19444, which keeps legacy setters through 2.x and removes them in 3.0; the delegated extractor API also documents removal in 3.0. Saying “next minor release” incorrectly tells users this API may disappear during 2.x. Please align the Javadoc and annotation comment with the 3.0 timeline.
* @deprecated Use {@link #setResponseHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
*/
@Deprecated // may be removed in the next minor release
instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/ServletTelemetryBuilder.java:149
- [General] The removal timeline conflicts with #19444, which keeps legacy setters through 2.x and removes them in 3.0; the delegated extractor API also documents removal in 3.0. Saying “next minor release” incorrectly tells users this API may disappear during 2.x. Please align the Javadoc and annotation comment with the 3.0 timeline.
* @deprecated Use {@link #setResponseHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
*/
@Deprecated // may be removed in the next minor release
instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v5_0/ServletTelemetryBuilder.java:149
- [General] The removal timeline conflicts with #19444, which keeps legacy setters through 2.x and removes them in 3.0; the delegated extractor API also documents removal in 3.0. Saying “next minor release” incorrectly tells users this API may disappear during 2.x. Please align the Javadoc and annotation comment with the 3.0 timeline.
* @deprecated Use {@link #setResponseHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
*/
@Deprecated // may be removed in the next minor release
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a99678d to
ff4986d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (8)
instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v5_0/ServletTelemetryBuilder.java:116
- The linked issue says deprecated Java setters remain through 2.x and are removed in 3.0, but this notice permits removal in the next minor release. Align the Javadoc and annotation with the 3.0 compatibility window.
* @deprecated Use {@link #setRequestHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v5_0/ServletTelemetryBuilder.java:147
- The linked issue says deprecated Java setters remain through 2.x and are removed in 3.0, but this notice permits removal in the next minor release. Align the Javadoc and annotation with the 3.0 compatibility window.
* @deprecated Use {@link #setResponseHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/ServletTelemetryBuilder.java:116
- The linked issue says deprecated Java setters remain through 2.x and are removed in 3.0, but this notice permits removal in the next minor release. Align the Javadoc and annotation with the 3.0 compatibility window.
* @deprecated Use {@link #setRequestHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/ServletTelemetryBuilder.java:147
- The linked issue says deprecated Java setters remain through 2.x and are removed in 3.0, but this notice permits removal in the next minor release. Align the Javadoc and annotation with the 3.0 compatibility window.
* @deprecated Use {@link #setResponseHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
instrumentation/restlet/restlet-2.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v2_0/RestletTelemetryBuilder.java:70
- The linked issue says deprecated Java setters remain through 2.x and are removed in 3.0, but this notice permits removal in the next minor release. Align the Javadoc and annotation with the 3.0 compatibility window.
* @deprecated Use {@link #setRequestHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
instrumentation/restlet/restlet-2.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v2_0/RestletTelemetryBuilder.java:101
- The linked issue says deprecated Java setters remain through 2.x and are removed in 3.0, but this notice permits removal in the next minor release. Align the Javadoc and annotation with the 3.0 compatibility window.
* @deprecated Use {@link #setResponseHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
instrumentation/restlet/restlet-1.1/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_1/RestletTelemetryBuilder.java:70
- The linked issue says deprecated Java setters remain through 2.x and are removed in 3.0, but this notice permits removal in the next minor release. Align the Javadoc and annotation with the 3.0 compatibility window.
* @deprecated Use {@link #setRequestHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
instrumentation/restlet/restlet-1.1/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_1/RestletTelemetryBuilder.java:101
- The linked issue says deprecated Java setters remain through 2.x and are removed in 3.0, but this notice permits removal in the next minor release. Align the Javadoc and annotation with the 3.0 compatibility window.
* @deprecated Use {@link #setResponseHeaders(IncludeExclude)} instead. May be removed in the next
* minor release.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Suppressed comments (4)
instrumentation/restlet/restlet-1.1/library/src/main/java/io/opentelemetry/instrumentation/restlet/v1_1/RestletTelemetryBuilder.java:106
- [General] This literal-matching guarantee is not currently preserved. The delegated
DefaultHttpServerInstrumenterBuilder#setCapturedResponseHeaderswraps these names in anIncludeExclude; with the new response-header enumeration, legacy values containing*or?are evaluated as globs. A legacy"*"value can therefore capture every response header rather than a literal header named*. Please add an exact-name path for deprecated setters and route this method through it.
* <p>The header names are matched literally. Unlike {@link #setResponseHeaders(IncludeExclude)},
* {@code *} and {@code ?} are not treated as glob patterns, since this setting never documented
* them as wildcards.
instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/ServletTelemetryBuilder.java:152
- [General] This literal-matching guarantee is not currently preserved. The delegated
DefaultHttpServerInstrumenterBuilder#setCapturedResponseHeaderswraps these names in anIncludeExclude; with the new response-header enumeration, legacy values containing*or?are evaluated as globs. A legacy"*"value can therefore capture every response header rather than a literal header named*. Please add an exact-name path for deprecated setters and route this method through it.
* <p>The header names are matched literally. Unlike {@link #setResponseHeaders(IncludeExclude)},
* {@code *} and {@code ?} are not treated as glob patterns, since this setting never documented
* them as wildcards.
instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v5_0/ServletTelemetryBuilder.java:152
- [General] This literal-matching guarantee is not currently preserved. The delegated
DefaultHttpServerInstrumenterBuilder#setCapturedResponseHeaderswraps these names in anIncludeExclude; with the new response-header enumeration, legacy values containing*or?are evaluated as globs. A legacy"*"value can therefore capture every response header rather than a literal header named*. Please add an exact-name path for deprecated setters and route this method through it.
* <p>The header names are matched literally. Unlike {@link #setResponseHeaders(IncludeExclude)},
* {@code *} and {@code ?} are not treated as glob patterns, since this setting never documented
* them as wildcards.
instrumentation/restlet/restlet-2.0/library/src/main/java/io/opentelemetry/instrumentation/restlet/v2_0/RestletTelemetryBuilder.java:106
- [General] This literal-matching guarantee is not currently preserved. The delegated
DefaultHttpServerInstrumenterBuilder#setCapturedResponseHeaderswraps these names in anIncludeExclude; with the new response-header enumeration, legacy values containing*or?are evaluated as globs. A legacy"*"value can therefore capture every response header rather than a literal header named*. Please add an exact-name path for deprecated setters and route this method through it.
* <p>The header names are matched literally. Unlike {@link #setResponseHeaders(IncludeExclude)},
* {@code *} and {@code ?} are not treated as glob patterns, since this setting never documented
* them as wildcards.
5b61fcd to
2b2c75c
Compare
28319d0 to
8d844c9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (1)
CHANGELOG.md:76
- [General] This release note conflicts with the PR description: it says legacy
*/?values are matched literally, while the description says they are dropped and produce a warning. The implementation delegates to the exact-name path (CapturedHttpHeaders.createExact), which performs literal lookups and does not log a warning. Please align the PR description with the implemented behavior, or implement the documented filtering and warning, so users receive unambiguous migration guidance.
exact name only. The deprecated methods still match header names literally, so `*` and `?` are not
treated as glob patterns.
Pull request dashboard statusMerged · refreshed 2026-08-18 21:11 UTC Status above doesn't look right?
|
8d844c9 to
a6e9021
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b99a9362-ef40-4f67-b2a0-5d6cab111d74
The deprecated setters accept exact header names, so converting them to an IncludeExclude selector reinterpreted those names as glob patterns. "*" is a legal HTTP header name character, so a caller who passed "*" previously looked up a header literally named "*" and captured nothing, and would have started capturing every header, including Authorization and Cookie. These settings never documented "*" as a wildcard, so globbing them widens capture rather than preserving behavior. They now delegate to the exact-name path on DefaultHttpServerInstrumenterBuilder, which matches literally. Also narrow ServletAccessor.getRequestHeaderNames to Collection<String>, matching the new getResponseHeaderNames, which drops an unreachable copy loop. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b99a9362-ef40-4f67-b2a0-5d6cab111d74
Passes "*" through the deprecated setters and asserts that no header is captured, so reintroducing the glob path fails loudly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b99a9362-ef40-4f67-b2a0-5d6cab111d74
The deprecated captured header setters match header names literally. The javadoc justified this by saying the setting never documented * and ? as wildcards, which concedes the behavior might have been wildcard-ish and relies on not having promised otherwise. The setting never supported them: HttpCommonAttributesExtractor looked each configured name up directly through the getter, lowercasing being the only transformation applied. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b99a9362-ef40-4f67-b2a0-5d6cab111d74
The regression test for passing "*" through the deprecated captured header setters only covered Servlet 3.0 and 5.0, even though the Restlet builders delegate to the same exact-name path and would regress the same way. Also drop a comment claiming a literal lookup of "*" finds nothing. Nothing is looked up: DeprecatedCaptureNames drops values containing "*" or "?" before the selector is built and logs a warning naming the replacement. What the fixture needs to say is why Authorization is there, which is that a glob would capture it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… capture tests
Review finding:
The x-secret exclusion assertions in capturesHeadersMatchingSelectors cannot fail. request.getHeaders("x-secret") and response.getHeaders("x-secret") are never stubbed, so the mocks return no values for that header. HttpCommonAttributesExtractor only puts a header attribute when the lookup returns a non-empty list, so http.request.header.x-secret and http.response.header.x-secret are absent whether or not the excluded pattern is honored, and the test would still pass if exclusion were broken entirely. The identical test in the Servlet 5.0 module has the same gap. Stub values for x-secret on both the request and the response so the doesNotContainKeys assertion actually exercises the exclusion.
Analysis: HttpCommonAttributesExtractor only records a header attribute when the getter returns a non-empty value list, so a header whose values are unstubbed can never produce an attribute. Both new servlet tests enumerated X-Secret through getHeaderNames() but stubbed values only for the allowed headers, which made the doesNotContainKeys assertions unfalsifiable. Stubbing x-secret values on the request and the response makes the excluded pattern the only thing that keeps those attributes out. Confirmed by temporarily changing the excluded pattern to a name that does not match: the assertion then failed with both x-secret attributes present, and it passes again with the pattern restored.
Upsides: The exclusion half of the new IncludeExclude selector semantics is now actually covered in both servlet modules, and a regression in exclude handling would fail the test instead of passing silently.
Downsides: No material downside identified.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f869d1c8-483d-4427-9f86-03401a8f0f25
a6e9021 to
466e5ab
Compare
|
|
||
| @Override | ||
| public Iterable<String> getRequestHeaderNames(HttpServletRequest httpServletRequest) { | ||
| public Collection<String> getRequestHeaderNames(HttpServletRequest httpServletRequest) { |
There was a problem hiding this comment.
I guess using Iterable instead of Collection could open possibilities for implementing this method without copying headers into a new list.
There was a problem hiding this comment.
Will bundle into a follow-up PR
The Restlet 1.1, Restlet 2.0, Servlet 3.0, and Servlet 5.0 library instrumentations can now select which HTTP headers to capture by glob pattern instead of by an exact list of names.
Matching semantics
?matches one character and*matches any number of characters, including none. Excluded patterns take precedence over included patterns, so a selector with no included patterns captures every header that is not excluded. Matching is case-insensitive, since HTTP header names are. No headers are captured when no selector is configured, or when the configured selector is empty.Header name enumeration
A wildcard or exclude-only selector has to know which headers a message actually carries, so it resolves names through
HttpCommonAttributesGetter#getHttpRequestHeaderNamesand#getHttpResponseHeaderNames, added in #19598. Restlet reads the names from the headerForm/Seriesattached to the message, and the servlet implementation lives once inservlet-common, since the getter is shared across servlet versions.Response header enumeration goes through a new
ServletAccessor.getResponseHeaderNamesmethod that defaults to an empty collection, becauseHttpServletResponse.getHeaderNames()only exists from Servlet 3.0 onward. The Servlet 3.0 and 5.0 accessors override it, so an exclude-only selector captures no response headers on Servlet 2.2 and on the container-specific accessors.Compatibility
setCapturedRequestHeadersandsetCapturedResponseHeadersonRestletTelemetryBuilder(1.1 and 2.0) andServletTelemetryBuilder(3.0 and 5.0) are deprecated in favor ofsetRequestHeaders(IncludeExclude)andsetResponseHeaders(IncludeExclude). They keep matching header names literally, so*and?in their values are not glob patterns: a configured*still captures only a header literally named*, which is nothing, and their javadoc says so. Moving a value to a selector is therefore a deliberate behavior change to review rather than a rename.Part of #19444.