switch alternative approach to declarative config#1060
switch alternative approach to declarative config#1060SylvainJuge merged 10 commits intoelastic:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughElasticAgent.main(...) now handles 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/entrypoint/src/main/java/co/elastic/otel/agent/ElasticAgent.java`:
- Around line 77-82: The code in ElasticAgent.printDefaultConfigYaml is loading
the resource at "inst/co/elastic/otel/config.yaml" which doesn't match the
packaged resource path "co/elastic/otel/config.yaml"; update the resource lookup
in printDefaultConfigYaml (the getResourceAsStream call and its String literal)
to use the packaged path "co/elastic/otel/config.yaml" (or resolve both paths
defensively) so --default-config-yaml can find and print the bundled config.
In
`@custom/src/test/java/co/elastic/otel/declarativeconfig/DefaultDeclarativeConfigTest.java`:
- Around line 63-67: In DefaultDeclarativeConfigTest, the four expected JSON
literals passed to json(...) are malformed with an extra closing brace; update
each call to use correctly balanced JSON strings (e.g., change
json("{\"process\":{}}}") to json("{\"process\":{}}") and likewise for
json("{\"container\":{}}}"), json("{\"service\":{}}}"), json("{\"host\":{}}}"),
ensuring the json(...) calls in the test class compile and parse the expected
values correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ca31295-8c04-4e64-832d-6d9324e7ab03
📒 Files selected for processing (5)
agent/entrypoint/src/main/java/co/elastic/otel/agent/ElasticAgent.javacustom/src/main/java/co/elastic/otel/declarativeconfig/ElasticDeclarativeConfigurationCustomizer.javacustom/src/main/resources/co/elastic/otel/config.yamlcustom/src/test/java/co/elastic/otel/declarativeconfig/DefaultDeclarativeConfigTest.javacustom/src/test/java/co/elastic/otel/declarativeconfig/ElasticDeclarativeConfigurationCustomizerTest.java
💤 Files with no reviewable changes (1)
- custom/src/main/java/co/elastic/otel/declarativeconfig/ElasticDeclarativeConfigurationCustomizer.java
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@custom/src/test/java/co/elastic/otel/declarativeconfig/DefaultDeclarativeConfigTest.java`:
- Around line 70-84: Remove the redundant outer assertThat wrappers around the
inner hasSize(1) calls: replace
assertThat(assertThat(config.getTracerProvider().getProcessors()).hasSize(1))
with assertThat(config.getTracerProvider().getProcessors()).hasSize(1), and
similarly replace the double-wrapped assertions for
config.getMeterProvider().getReaders() and
config.getLoggerProvider().getProcessors() in DefaultDeclarativeConfigTest (use
the direct assertThat(...).hasSize(1) form so the size assertion is the actual
assertion).
- Around line 76-88: The test is asserting wrong exporter paths for meters and
logs: update the inPath lookups in DefaultDeclarativeConfigTest to match the
configured otlp_grpc exporter; specifically, when asserting
config.getMeterProvider().getReaders().get(0) use
inPath("periodic.exporter.otlp_grpc") and when asserting
config.getLoggerProvider().getProcessors().get(0) use
inPath("batch.exporter.otlp_grpc") so the endpoint assertions target the actual
exporters defined in the YAML.
In
`@smoke-tests/src/test/java/com/example/javaagent/smoketest/DeclarativeConfigSmokeTest.java`:
- Around line 41-52: The test launches a child JVM (Process created in
DeclarativeConfigSmokeTest) and on waitFor timeout currently throws without
killing the subprocess; update the exit/timeout handling to destroy the process
when process.waitFor(5, TimeUnit.SECONDS) returns false or when
process.exitValue() != 0 (call process.destroyForcibly() and/or
process.destroy() as appropriate) before throwing the IllegalStateException, and
also ensure the catch block for InterruptedException/IOException destroys the
process if it was started; locate the Process variable created by new
ProcessBuilder(...).start() and add safe teardown (destroy/destroyForcibly and
optionally waitFor after destroy) to prevent leaking child JVMs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f286abf-191a-4dc1-adbb-2c0d99984efa
📒 Files selected for processing (6)
custom/src/main/java/co/elastic/otel/declarativeconfig/ElasticDeclarativeConfigurationCustomizer.javacustom/src/main/resources/co/elastic/otel/config.yamlcustom/src/test/java/co/elastic/otel/declarativeconfig/DefaultDeclarativeConfigTest.javasmoke-tests/src/test/java/com/example/javaagent/smoketest/DeclarativeConfigSmokeTest.javasmoke-tests/src/test/java/com/example/javaagent/smoketest/SmokeTest.javasmoke-tests/src/test/java/com/example/javaagent/smoketest/TestAppSmokeTest.java
✅ Files skipped from review due to trivial changes (1)
- custom/src/main/resources/co/elastic/otel/config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- custom/src/main/java/co/elastic/otel/declarativeconfig/ElasticDeclarativeConfigurationCustomizer.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@smoke-tests/src/test/java/com/example/javaagent/smoketest/SmokeTest.java`:
- Around line 387-391: getResourceAttributes currently collects flattened
KeyValue pairs into a Map using Collectors.toMap which throws
IllegalStateException on duplicate keys (e.g., repeated "service.name"); update
getResourceAttributes to handle duplicate keys by supplying a merge function to
Collectors.toMap (e.g., choose first or last value) and optionally use a
deterministic map supplier (like LinkedHashMap) to preserve order; locate
getResourceAttributes and the KeyValue::getKey / KeyValue::getValue usage and
replace the collector with Collectors.toMap(KeyValue::getKey,
KeyValue::getValue, (v1,v2)->v1, LinkedHashMap::new) or an equivalent merge
strategy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46ab1fd3-8f8a-4e83-ad8b-6472868ae898
📒 Files selected for processing (5)
custom/src/main/resources/co/elastic/otel/config.yamlcustom/src/test/java/co/elastic/otel/declarativeconfig/DefaultDeclarativeConfigTest.javasmoke-tests/src/test/java/com/example/javaagent/smoketest/DeclarativeConfigSmokeTest.javasmoke-tests/src/test/java/com/example/javaagent/smoketest/JavaExecutable.javasmoke-tests/src/test/java/com/example/javaagent/smoketest/SmokeTest.java
✅ Files skipped from review due to trivial changes (1)
- custom/src/main/resources/co/elastic/otel/config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- smoke-tests/src/test/java/com/example/javaagent/smoketest/DeclarativeConfigSmokeTest.java
| public void customize(DeclarativeConfigurationCustomizer customizer) { | ||
| customizer.addModelCustomizer( | ||
| model -> { | ||
| if (model == null) { |
There was a problem hiding this comment.
[for reviewer] this just prevents a confusing NPE when the configuration can't be parsed, hence making it harder to debug for the end-user.
|
|
||
| customizeResources(model); | ||
| customizeUserAgent(model); | ||
| customizeExperimentalRuntimeTelemetryMetrics(model); |
There was a problem hiding this comment.
[for reviewer] we remove the "magic" implementation, this is replaced with the default config in yaml (unfortunately we don't have a test for it, at least yet).
| // For now, we can't override env variable for testing, thus we just verify the default values | ||
| // in the configuration we provide. | ||
| @Test | ||
| void testDefaults() { |
There was a problem hiding this comment.
[for reviewer] this tests that the yaml has a correct structure and contains the things we hope it contains, it does not really checks that everything in it is actually correct, which is what the smoke test provides.
| } | ||
|
|
||
| @Test | ||
| void optOutExperimentalRuntimeMetrics() { |
There was a problem hiding this comment.
[for reviewer] I'm happy to get rid of this "half hadooken code"
2 participants
part of #1054
--default-config-yamlCLI option to access the embedded yaml configuration in agent jar.possible follow-up: