-
Notifications
You must be signed in to change notification settings - Fork 179
[dynamic control] Add pipeline config initialization from file #2753
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
Open
jackshirazi
wants to merge
2
commits into
open-telemetry:main
Choose a base branch
from
jackshirazi:policy23
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+217
−0
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
156 changes: 156 additions & 0 deletions
156
.../src/test/java/io/opentelemetry/contrib/dynamic/policy/registry/PolicyInitConfigTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.contrib.dynamic.policy.registry; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import io.opentelemetry.contrib.dynamic.policy.source.SourceFormat; | ||
| import io.opentelemetry.contrib.dynamic.policy.source.SourceKind; | ||
| import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.io.TempDir; | ||
|
|
||
| class PolicyInitConfigTest { | ||
|
|
||
| @TempDir Path tempDir; | ||
|
|
||
| @Test | ||
| void readFromConfigPropertiesReturnsNullWhenBothPathsMissing() { | ||
| ConfigProperties config = mock(ConfigProperties.class); | ||
| when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_YAML)).thenReturn(null); | ||
| when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_JSON)).thenReturn(null); | ||
|
|
||
| assertThat(PolicyInitConfig.readFromConfigProperties(config)).isNull(); | ||
| } | ||
|
|
||
| @Test | ||
| void readFromConfigPropertiesPrefersYamlWhenBothPathsPresent() throws Exception { | ||
| Path jsonPath = tempDir.resolve("policy-init.json"); | ||
| Files.write(jsonPath, jsonWithLocation("from-json").getBytes(StandardCharsets.UTF_8)); | ||
| Path yamlPath = tempDir.resolve("policy-init.yaml"); | ||
| Files.write(yamlPath, yamlWithLocation("from-yaml").getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| ConfigProperties config = mock(ConfigProperties.class); | ||
| when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_YAML)) | ||
| .thenReturn(yamlPath.toString()); | ||
| when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_JSON)) | ||
| .thenReturn(jsonPath.toString()); | ||
|
|
||
| PolicyInitConfig initConfig = PolicyInitConfig.readFromConfigProperties(config); | ||
|
|
||
| assertThat(initConfig).isNotNull(); | ||
| assertThat(initConfig.getSources()).hasSize(1); | ||
| assertThat(initConfig.getSources().get(0).getLocation()).isEqualTo("from-yaml"); | ||
| } | ||
|
|
||
| @Test | ||
| void readFromConfigPropertiesFallsBackToJsonWhenYamlBlank() throws Exception { | ||
| Path jsonPath = tempDir.resolve("policy-init.json"); | ||
| Files.write(jsonPath, jsonWithLocation("from-json").getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| ConfigProperties config = mock(ConfigProperties.class); | ||
| when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_YAML)).thenReturn(" "); | ||
| when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_JSON)) | ||
| .thenReturn(jsonPath.toString()); | ||
|
|
||
| PolicyInitConfig initConfig = PolicyInitConfig.readFromConfigProperties(config); | ||
|
|
||
| assertThat(initConfig).isNotNull(); | ||
| assertThat(initConfig.getSources()).hasSize(1); | ||
| assertThat(initConfig.getSources().get(0).getLocation()).isEqualTo("from-json"); | ||
| } | ||
|
|
||
| @Test | ||
| void readFromConfigPropertiesReadsJsonWhenYamlPathMissing() throws Exception { | ||
| Path configPath = tempDir.resolve("policy-init.json"); | ||
| Files.write(configPath, minimalJsonConfig().getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| PolicyInitConfig config = configFromPaths(null, configPath.toString()); | ||
|
|
||
| assertThat(config.getSources()).hasSize(1); | ||
| PolicySourceConfig source = config.getSources().get(0); | ||
| assertThat(source.getKind()).isEqualTo(SourceKind.OPAMP); | ||
| assertThat(source.getFormat()).isEqualTo(SourceFormat.JSONKEYVALUE); | ||
| assertThat(source.getLocation()).isEqualTo("vendor"); | ||
| assertThat(source.getMappings()).hasSize(1); | ||
| assertThat(source.getMappings().get(0).getSourceKey()).isEqualTo("sampling_rate"); | ||
| assertThat(source.getMappings().get(0).getPolicyType()).isEqualTo("trace_sampling_rate_policy"); | ||
| } | ||
|
|
||
| @Test | ||
| void readFromConfigPropertiesReadsYamlWhenYamlPathProvided() throws Exception { | ||
| Path configPath = tempDir.resolve("policy-init.yaml"); | ||
| Files.write(configPath, minimalYamlConfig().getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| PolicyInitConfig config = configFromPaths(configPath.toString(), null); | ||
|
|
||
| assertThat(config.getSources()).hasSize(1); | ||
| PolicySourceConfig source = config.getSources().get(0); | ||
| assertThat(source.getKind()).isEqualTo(SourceKind.OPAMP); | ||
| assertThat(source.getFormat()).isEqualTo(SourceFormat.JSONKEYVALUE); | ||
| assertThat(source.getMappings()).hasSize(1); | ||
| } | ||
|
|
||
| @Test | ||
| void readFromConfigPropertiesReturnsNullOnFileReadFailure() { | ||
| Path missing = tempDir.resolve("missing-policy-init.json"); | ||
|
|
||
| assertThat(configFromPaths(null, missing.toString())).isNull(); | ||
| } | ||
|
|
||
| @Test | ||
| void readFromConfigPropertiesReturnsNullOnParseFailure() throws Exception { | ||
| Path badJson = tempDir.resolve("bad-policy-init.json"); | ||
| Files.write(badJson, "{}".getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| assertThat(configFromPaths(null, badJson.toString())).isNull(); | ||
| } | ||
|
|
||
| private static PolicyInitConfig configFromPaths(String yamlPath, String jsonPath) { | ||
| ConfigProperties config = mock(ConfigProperties.class); | ||
| when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_YAML)).thenReturn(yamlPath); | ||
| when(config.getString(PolicyInitConfig.POLICY_INIT_CONFIG_PROPERTY_JSON)).thenReturn(jsonPath); | ||
| return PolicyInitConfig.readFromConfigProperties(config); | ||
| } | ||
|
|
||
| private static String minimalJsonConfig() { | ||
| return "{\"sources\":[{\"kind\":\"opamp\",\"format\":\"jsonkeyvalue\",\"location\":\"vendor\"," | ||
| + "\"mappings\":[{\"sourceKey\":\"sampling_rate\",\"policyType\":\"trace_sampling_rate_policy\"}]}]}"; | ||
| } | ||
|
|
||
| private static String jsonWithLocation(String location) { | ||
| return "{\"sources\":[{\"kind\":\"opamp\",\"format\":\"jsonkeyvalue\",\"location\":\"" | ||
| + location | ||
| + "\",\"mappings\":[{\"sourceKey\":\"sampling_rate\",\"policyType\":\"trace_sampling_rate_policy\"}]}]}"; | ||
| } | ||
|
|
||
| private static String minimalYamlConfig() { | ||
| return "sources:\n" | ||
| + " - kind: opamp\n" | ||
| + " format: jsonkeyvalue\n" | ||
| + " location: vendor\n" | ||
| + " mappings:\n" | ||
| + " - sourceKey: sampling_rate\n" | ||
| + " policyType: trace_sampling_rate_policy\n"; | ||
| } | ||
|
|
||
| private static String yamlWithLocation(String location) { | ||
| return "sources:\n" | ||
| + " - kind: opamp\n" | ||
| + " format: jsonkeyvalue\n" | ||
| + " location: " | ||
| + location | ||
| + "\n" | ||
| + " mappings:\n" | ||
| + " - sourceKey: sampling_rate\n" | ||
| + " policyType: trace_sampling_rate_policy\n"; | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
can we use declarative config api instead? (and rely on the java agent declarative config bridge for the system property support)
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.
I'm not sure that gives any benefit. The system property doesn't provide the pipeline configuration, it points to a file that provides pipeline configuration. We'd need to change the property to be an ugly string encoded yaml to use that. json would work because that can be a single string for the full config, but yaml would be ugly.
Even to define a small pipeline this is painful
The yaml is deliberately setup so that when the telemetry policy otep is accepted and we then add
policyto declarative config, it slots right in, egThe node under policy is exactly what this is supporting. To use the declarative config api just to get the file path seems pointless?
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.
does it make sense to slot it in now, even if the location under declarative config changes later when it gets an official location?
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.
We can do that for the declarative config, since this is experimental it would be fine. But how would you define the config when declarative config is not used.
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.
could this be a declarative config only feature? (similar to rule based sampling, some of the new method instrumentation, ..)
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.
Allowing the structure to be read by failing over to how this PR does it would work. So it would look like this. I need to add the additional methods if this plan looks okay
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.
I've done a different implementation in #2766 (I'll close this PR when this discussion is complete or shifts to that).
I think having only declarative config supported imposes a very high burden here. You already need to include this as an extension, insisting that you must also wire everything up using declarative config in order to use this makes it a huge shift instead of the smaller one I'd like to support
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.
yeah, I understand. at the same time, the whole reason we (in Java at least) have been looking forward to declarative configuration is that it allows us to have a unified story for these kinds of complex configurations, instead of having lots of different config files that are pointed to by various configuration properties (e.g. having separate metric view yaml configuration file, rule-based sampling configuration file, jmx configuration file, opamp policy configuration file, methods instrumentation configuration file, etc etc..)
Uh oh!
There was an error while loading. Please reload this page.
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.
I've made the declarative config the default in the new PR, with support for experimental config files only if declarative config is not used.
Without the config file failover, it just forces deployments that don't use declarative config to use another coded extension to create the pipeline which seems like imposing a lot of additional pain to avoid including an option and a method