Repository navigation
v2.33.0 post release changes - #3148
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
| map, | ||
| new TextMapGetter<>() { | ||
| @Override | ||
| public Iterable<String> keys(Map<String, String> carrier) { |
There was a problem hiding this comment.
the keys method is deprecated and now has a default implementation that returns and empty iterable
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97108911c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import java.util.logging.Logger; | ||
|
|
||
| @AutoService(ComponentProvider.class) | ||
| public class TruncateCommandLineResourceDetector implements ComponentProvider { |
There was a problem hiding this comment.
removed this for now since user would need to manually enable it
There was a problem hiding this comment.
Hmm, I'm still not sure if removing it is a good solution, but maybe nobody use it...
If we leave the code and let user to add truncate_command_line_process manually to the declarative config YAML, then is it very different from setting some property in YAML instead?
There was a problem hiding this comment.
As far as I understand the problematic command args arguments are disabled by default and currently it is impossible to enable them in the declarative configuration so we don't need to truncate them. In my opinion we don't need to do anything unless someone requests a way to enable these attributes. In that case we'd first need to figure out a way to enable them in upstream and after that we'd need to either truncate them in our distro or emit a warning.
There was a problem hiding this comment.
The issue I see is that for envvars config user may enable cmd-line arguments reporting by setting otel.instrumentation.resources.experimental.process-command-attributes.enabled (read by ProcessResourceProvider), but for declarative config it will be impossible. As you pointed out - there is no corresponding mechanism for declarative config yet, but if we leave this class then user will be able to specify appropriate resource detector to have very similar effect to envvar:
processto get a process resource without cmd argstruncate_command_line_processto get a process resource with truncated cmd args included
I think we don't have to enable it in the upstream first. We can remove our class later once upstream solves declarative config issue with opt-in enabling cmd line args reporting.
Anyway, let's move forward with removing it now, if you think it is OK solution.
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
| // This class is loaded in the insolated instrumentation class loaded we expose it to | ||
| // application class loader so that tests can temporarily disable instrumentation to read | ||
| // tracing state. | ||
| return Boolean.getBoolean("jdbcTestMode") |
There was a problem hiding this comment.
This is needed because of switching to non-inline advice. An alternative is to use reflection in the test to get access to the isolated instrumentation class loader and load the class from there. That approach is used in a couple of upstream tests.
| import java.util.logging.Logger; | ||
|
|
||
| @AutoService(ComponentProvider.class) | ||
| public class TruncateCommandLineResourceDetector implements ComponentProvider { |
There was a problem hiding this comment.
Hmm, I'm still not sure if removing it is a good solution, but maybe nobody use it...
If we leave the code and let user to add truncate_command_line_process manually to the declarative config YAML, then is it very different from setting some property in YAML instead?
| val otelVersion = "1.67.0" | ||
| val otelAlphaVersion = otelVersion.replaceFirst("(-SNAPSHOT)?$".toRegex(), "-alpha$1") | ||
| val otelInstrumentationVersion = "2.33.0" | ||
| val otelInstrumentationVersion = "3.0.0-SNAPSHOT" |
There was a problem hiding this comment.
We should decide if we want to do a 2.33.1 patch before merging this I suppose.
There was a problem hiding this comment.
no, patch releases are made from a branch as described in the release instructions. For further minor releases for 2.x series we'll need to create a separate branch similarly to upstream maintenance/v2.x branch and update the release instructions.
v2.33.0 post release changes