[File based config] Support for Appd#2498
Conversation
This reverts commit 2564119.
| @Override | ||
| public void beforeAgent(AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk) { | ||
| ConfigProperties config = getConfig(autoConfiguredOpenTelemetrySdk); | ||
| if (!config.getBoolean("cisco.ctx.enabled", false)) { |
There was a problem hiding this comment.
[for reviewer] Because we need to check "cisco.ctx.enabled" here, there was no point in getting rid of AppdBonusConfigurationCustomizerProvider class which also checks this property and adds appropriate propagator and span processor.
| @SuppressWarnings("rawtypes") | ||
| @AutoService(ComponentProvider.class) | ||
| public class AppdBonusSpanProcessorComponentProvider implements ComponentProvider<SpanProcessor> { | ||
| static final String NAME = "appd-bonus"; |
There was a problem hiding this comment.
[for reviewer] YAML naming convention prefers using underscore, but for consistency with environment variables based config I used hyphen in the name.
| Resource resource = getResource(autoConfiguredOpenTelemetrySdk); | ||
| logger.fine(() -> "Setting up AppdBonusPropagator with resource: " + resource); | ||
|
|
||
| AppdBonusPropagator appdBonusPropagator = AppdBonusPropagator.getInstance(); |
There was a problem hiding this comment.
[for reviewer] This will also run when environment variables based configuration is in use. In such a case these propagator's properties are already set by AppdBonusCustomizer. If you think this is an issue I can cleanup AppdBunusCustomizer . This may make sense only if AppdBonusPropagator and span processor are long term solution, not a temporary one.
...rc/test/java/com/splunk/opentelemetry/appd/AppdBonusConfigurationCustomizerProviderTest.java
Show resolved
Hide resolved
|
|
||
| private static boolean isFeatureEnabled( | ||
| OpenTelemetryConfigurationModel model, Map<String, Object> properties) { | ||
| return (getAdditionalPropertyOrDefault(properties, CONFIG_CISCO_CTX_ENABLED, false)); |
| compositeList = | ||
| compositeList.isEmpty() | ||
| ? AppdBonusPropagator.NAME | ||
| : AppdBonusPropagator.NAME + "," + compositeList; |
There was a problem hiding this comment.
Without declarative config we need to add the default propagators. Just checking whether this is also needed here or is it so that when you don't add propagators to yaml then you don't get any default ones?
There was a problem hiding this comment.
Good catch! I missed that. Fixed now
breedx-splk
left a comment
There was a problem hiding this comment.
Had a couple small comments, but looks good to me.
...om/src/main/java/com/splunk/opentelemetry/appd/AppdBonusConfigurationCustomizerProvider.java
Outdated
Show resolved
Hide resolved
| if (compositeList == null) { | ||
| compositeList = ""; | ||
| } else if (!canAddPropagator(compositeList)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The else makes it unnecessarily complex. In the null case, canAddPropagator() will return false anyway.
| if (compositeList == null) { | |
| compositeList = ""; | |
| } else if (!canAddPropagator(compositeList)) { | |
| return false; | |
| } | |
| if (compositeList == null) { | |
| compositeList = ""; | |
| } | |
| if (!canAddPropagator(compositeList)) { | |
| return false; | |
| } |
There was a problem hiding this comment.
I did even more refactoring and then merged it. Let me know if you don't like it. I can always create another PR
Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
…a into fbc-appd-bonus
This is support for AppD context. It includes service name and deployment environment name, as well as business transaction related data.
To make it working use the following snippet:
This implementation utilizes BeforeAgentListener to copy service name and deployment environment name from resource attributes to corresponding fields of Appd context propagator. It is implemented this way for two main reasons: