-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(EUM-server): Add conversion-service to EUM server #1346
fix(EUM-server): Add conversion-service to EUM server #1346
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aaronweissler)
components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/beacon/BeaconHttpExporter.java, line 22 at r1 (raw file):
@Slf4j @ConditionalOnProperty({"inspectit-eum-server.exporters.beacons.http.enabled"}) @ConditionalOnExpression("NOT new String('${inspectit-eum-server.exporters.beacons.http.enabled}').toUpperCase().equals(T(rocks.inspectit.ocelot.config.model.exporters.ExporterEnabledState).DISABLED.toString())")
why do we need to convert it to toUpperCase()
?
Is the value parsed from deprecated configurations disabled
, DISABLED
(according to BooleanToExporterEnabledStateConverter.java
), or false
?
We could potentially also try to parse the ExporterEnabledState
from the String and checker whether it is .disabled()
, i.e. ExporterEnabledState.valueOf(new String('${inspectit-eum-server.exporters.beacons.http.enabled}')).isDisabled()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @heiko-holz)
components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/beacon/BeaconHttpExporter.java, line 22 at r1 (raw file):
Previously, heiko-holz (Heiko Holz) wrote…
why do we need to convert it to
toUpperCase()
?
Is the value parsed from deprecated configurationsdisabled
,DISABLED
(according toBooleanToExporterEnabledStateConverter.java
), orfalse
?We could potentially also try to parse the
ExporterEnabledState
from the String and checker whether it is.disabled()
, i.e.ExporterEnabledState.valueOf(new String('${inspectit-eum-server.exporters.beacons.http.enabled}')).isDisabled()
In both cases the problem is that this evaluation seems to happen really early on the raw input data before Spring uses the converters or even parses it.
The toUpperCase() is then needed, because someone could put in "disabled" as the enabled
value which Spring would accept just fine and later parse as ExporterEnabledState.DISABLED, but in the evaluation it is still seen as "disabled" which would lead to the Component being built despite actually being supposed to be disabled. This part is a correction for the new correct way of setting enabled
.
The deprecated way would be setting it to false, which is filtered by the ConditionalOnProperty
annotation already.
This early evaluation also makes the recommendation impossible unfortunately, because if the old deprecated way is used this would throw an Exception, since it would try to parse "false" or "true" with valueOf, because they will only get converted correctly afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aaronweissler)
components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/exporters/beacon/BeaconHttpExporter.java, line 22 at r1 (raw file):
Previously, aaronweissler wrote…
In both cases the problem is that this evaluation seems to happen really early on the raw input data before Spring uses the converters or even parses it.
The toUpperCase() is then needed, because someone could put in "disabled" as the
enabled
value which Spring would accept just fine and later parse as ExporterEnabledState.DISABLED, but in the evaluation it is still seen as "disabled" which would lead to the Component being built despite actually being supposed to be disabled. This part is a correction for the new correct way of settingenabled
.The deprecated way would be setting it to false, which is filtered by the
ConditionalOnProperty
annotation already.This early evaluation also makes the recommendation impossible unfortunately, because if the old deprecated way is used this would throw an Exception, since it would try to parse "false" or "true" with valueOf, because they will only get converted correctly afterwards.
thanks for the exhaustive explanation!
* Closes #1291: Extend the Configuration UI to display Configuration Documentation (#1317) Co-authored-by: Marius Oehler <[email protected]> * Minor documentation fix (#1328) * Closes #1318 - Only accept Maps as Config-Yaml content (#1320) * Closes #1319 - Add filter for inspectit.env path in autocompletion (#1339) * Add filter for inspectit.env path in autocompletion * Update tests * Closes #1329 - Fix parsing of Config-Yaml with unknown properties for Config-Docs (#1330) * Closes #1159 - Close resources (#1160) Co-authored-by: Marius Oehler <[email protected]> * Closes #1284 - Agent command allows retrieving the latest logs (#1332) * added logs method - actual functionality missing * safe for the day * added extractlogsappender * add test case for config loading, basic fixes, and comments * externalise preloaded logs into LogPreloader and add tests; rename ExtractLogsAppender into LogPreloadingAppender * add comments what to do next * add test case describing expected behavior of LogsCommandExecutor * edited configuration for ´log output' * debug prototype finished * filter preloaded messages by log level and highlight open issues * make the log preloader a DynamicallyActivatableService * change config path to inspectit.log-preloading * make logback initializer react to changes to self-monitoring or log-preloading * format logs and rename command result attribute to logs * adjust and fix test cases * add default configuration and documentation * cosmetic fixes * further cosmetic stuff * use correct mockito method * add validation annotations to log preloading settings * Merge branch 'master' into ocelot_debug_feature * Closes #1248 - Refactor enabled field for exporters (#1303) * Disable all exporters by default * Update Documentation * Test logging warnings * Refactor enabled into enum * Set log-level to warning * Add changes to EUM server * Unifrom javadoc for enabled field * Add converter to support old configurations * Fix log-level in EUM server * Update breaking-changes.md * Small fix to JavaDoc * Re-add Info log * Fix Attachment Test * Fix agent_test.yml * Fix agent_test.yml * feat(exporter): [#1248] Minor refactoring of ExporterEnabledState.java and related exporter services; minor text/comment adjustment * Small fixes to documentation * Fix documentation in default config * Fix agent_test.yml * Debug attachment test * Debug attachment test * Debug attachment test * Fix Attachment Test * Remove Attachment Test debugging code * Add Documentation to Converter * Small fixes to Documentation * Simplify tests * feat(exporter): [#1248] small refactor in `*ExporterServiceIntTest.java` * Test TraceExportersConfiguration Annotations * Fix TraceExportersConfiguration Test * Fix EumExportersSettings * Update TraceExportersConfigurationTest.java * Added some Docs explaining the new test * Add docs for metrics field in EumExportersSettings Co-authored-by: Heiko Holz <[email protected]> * feat(ui): reload documentation when configuration files are changed (#1340) Refs: #1288 * feat(ui): ability to filter configuration docs (#1341) * [skip ci] Publish documentation v1.15.0 * fix(config-docs) - Handle old Boolean style for enabled property (#1343) Co-authored-by: Marius Oehler <[email protected]> * [skip ci] Publish documentation v1.15.1 * fix(EUM-server): update Beacon Exporter enabled (#1344) * fix(EUM-Server): Addition to Beacon exporter fix (#1345) * fix(EUM-Server): Addition to Beacon exporter fix * Small fixes to other exporter tests * Add test for beacon exporter-related annotations * fix(EUM-server): Add conversion-service to EUM server (#1346) * Add conversionService to EUM-Server * Refactor Conditional annotations * Merge remote-tracking branch 'origin/master' into feature/opentelemetry-migration # Conflicts: # components/inspectit-ocelot-configurationserver/src/test/java/rocks/inspectit/ocelot/autocomplete/autocompleterimpl/ModelAutoCompleterTest.java # inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterService.java # inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentMetricsExporterService.java # inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentTraceExporterService.java # inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/PrometheusExporterService.java # inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/exporter/ZipkinExporterService.java # inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/JaegerExporterServiceIntTest.java # inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentMetricsExporterServiceIntTest.java # inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/OpenCensusAgentTraceExporterServiceIntTest.java # inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/PrometheusExporterServiceIntTest.java # inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/ZipkinExporterServiceIntTest.java # inspectit-ocelot-documentation/docs/metrics/metric-exporters.md # inspectit-ocelot-documentation/docs/tracing/trace-exporters.md
This change is