Metrics: Agent settings prefix telemetry.agent preferred over tracing.apm.agent by stu-elastic · Pull Request #104345 · elastic/elasticsearch

stu-elastic · 2024-01-12T23:07:48Z

Prefer the telemetry.agent prefix for APM agent settings.

Add a fallback prefix to Affix settings to migrating between an old prefix and a new prefix.

….agent Prefer the telemetry.agent prefix for APM agent settings. Add a fallback prefix to Affix settings to migrating between an old prefix and a new prefix.

elasticsearchmachine · 2024-01-12T23:08:12Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

pgomulka

it looks good, I left comments

pgomulka · 2024-01-15T08:46:27Z

...ribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java

+                allOf(
+                    hasEntry("server_url", "https://myurl:443"),
+                    hasEntry("service_node_name", "instance-0000000001"),
+                    hasEntry(equalTo("global_labels"), not(endsWith(","))), // test that we have collapsed all global labels into one


why not asserting about the exact entry?

@pgomulka See below

List<String> labels = Arrays.stream(extracted.get("global_labels").split(",")).toList(); assertThat(labels, hasSize(3)); assertThat(labels, containsInAnyOrder("deployment_name=APM Tracing", "organization_id=456", "deployment_id=123"));

containsInAnyOrder with 3 args is implying the size 3. but I think it looks

pgomulka · 2024-01-15T08:50:49Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

-        }
-
-        AffixKey(String prefix, String suffix) {
+        AffixKey(String prefix, String suffix, String fallbackPrefix) {


should we call deprecation logger when a fallback prefix was used?

I'm moving this and related changes to another PR because it would be a backwards incompatible change.

distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/APMJvmOptions.java

mosche · 2024-01-15T10:06:23Z

...ribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java

+                allOf(
+                    hasEntry("server_url", "https://myurl:443"),
+                    hasEntry("service_node_name", "instance-0000000001"),
+                    hasEntry(equalTo("global_labels"), not(endsWith(","))), // test that we have collapsed all global labels into one


@pgomulka See below

List<String> labels = Arrays.stream(extracted.get("global_labels").split(",")).toList(); assertThat(labels, hasSize(3)); assertThat(labels, containsInAnyOrder("deployment_name=APM Tracing", "organization_id=456", "deployment_id=123"));

...ribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java

server/src/main/java/org/elasticsearch/common/settings/Setting.java

…refix for unknown settings in APMAgentSettingsTests.testRejectForbiddenOrUnknownSettings

stu-elastic · 2024-01-17T15:24:39Z

I'm addressing the testPrefixKeySettingFallbackAsMap failure, we should prefer the new prefix over the old prefix in getAsMap.

pgomulka

LGTM

pgomulka · 2024-01-17T15:24:33Z

...ribution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/APMJvmOptionsTests.java

+                allOf(
+                    hasEntry("server_url", "https://myurl:443"),
+                    hasEntry("service_node_name", "instance-0000000001"),
+                    hasEntry(equalTo("global_labels"), not(endsWith(","))), // test that we have collapsed all global labels into one


containsInAnyOrder with 3 args is implying the size 3. but I think it looks

pgomulka · 2024-01-17T15:29:29Z

distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/APMJvmOptions.java

-        agentSettings.keySet().forEach(key -> propertiesMap.put(key, String.valueOf(agentSettings.get(key))));
+        // tracing.apm.agent. is deprecated by telemetry.agent.
+        final String telemetryAgentPrefix = "telemetry.agent.";
+        final String tracingAgentPrefix = "tracing.apm.agent.";


i wonder if it would be readable by naming those two telemetryAgentPrefix and deprecatedTelemetryAgentPrefix

stu-elastic · 2024-01-17T23:41:42Z

we should prefer the new prefix over the old prefix in getAsMap.

That was a bit much for the implementation in getAsMap, duplicate settings are applied in any order already so I removed that part of the test.

mosche · 2024-01-18T05:56:02Z

server/src/test/java/org/elasticsearch/common/settings/SettingTests.java

+
+        map = setting.getAsMap(Settings.builder().put("bar.bar", "true").build());
+        assertEquals(1, map.size());
+        assertTrue(map.get("bar"));


we should prefer the new prefix over the old prefix in getAsMap.

That was a bit much for the implementation in getAsMap, duplicate settings are applied in any order already so I removed that part of the test.

map = setting.getAsMap(Settings.builder().put("foo.bar", "false").put("bar.bar", "true").build()); assertEquals(1, map.size()); assertFalse(map.get("bar"));

I'm a bit worried about this not working, you might end up using a fallback rather than the correct setting value without noticing. Wouldn't something like below work, I haven't tested it. But the idea is, if the map already contains a value for namespace, only ever overwrite it the key is prefixed with the default prefix and not the fallback one.

public Map<String, T> getAsMap(Settings settings) { Map<String, T> map = new HashMap<>(); matchStream(settings).distinct().forEach(key -> { String namespace = this.key.getNamespace(key); if (map.containsKey(namespace) == false || key.startsWith(this.key.prefix)) { Setting<T> concreteSetting = getConcreteSetting(namespace, key); map.put(namespace, concreteSetting.get(settings)); } }); return Collections.unmodifiableMap(map); }

I was under the mistaken impression that in normal settings the primary was not preferred, so it didn't make sense to add that behavior here. I confirmed in the case of a primary setting and it's fallback, the primary is preferred. Added a test to that effect.

I've also made AffixSettings match that behavior and restored the quoted test case.

…y-agent-fallback

mosche

LGTM 🎉

stu-elastic added 2 commits January 12, 2024 17:03

Metrics: Agent settings prefix telemetry.agent deprecates tracing.apm…

f1df1cb

….agent Prefer the telemetry.agent prefix for APM agent settings. Add a fallback prefix to Affix settings to migrating between an old prefix and a new prefix.

remove extra files

ddb7340

stu-elastic added >non-issue :Core/Infra/Core Core issues without another label labels Jan 12, 2024

stu-elastic requested review from mosche and pgomulka January 12, 2024 23:07

elasticsearchmachine added v8.13.0 Team:Core/Infra Meta label for core/infra team labels Jan 12, 2024

pgomulka reviewed Jan 15, 2024

View reviewed changes

mosche reviewed Jan 15, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/common/settings/Setting.java Outdated Show resolved Hide resolved

stu-elastic added 5 commits January 16, 2024 15:29

Add telemetry as preferred prefix in APMAgentSettings, test correct p…

1dd031c

…refix for unknown settings in APMAgentSettingsTests.testRejectForbiddenOrUnknownSettings

error if telemetry labels use both prefixes

45b3fae

spotless on APMJvmOptionsTests

71c23f2

deprecate fallback prefix in Affix settings

36cb3d4

cleanup repeated settings

d5b0113

stu-elastic requested review from mosche and pgomulka January 17, 2024 01:18

spotless

58dbb02

pgomulka mentioned this pull request Jan 17, 2024

Fix enabling / disabling of APM agent "recording" in APMAgentSettings #104324

Merged

pgomulka approved these changes Jan 17, 2024

View reviewed changes

update AffixKey hascode and equals, do not deduplicate affix settings

e1efd97

stu-elastic added 2 commits January 17, 2024 17:42

tracingAgentPrefix -> deprecatedTelemetryAgentPrefix

e7946be

use namespace delegate factory for fallback prefix affixsetting

8cbd40e

mosche reviewed Jan 18, 2024

View reviewed changes

stu-elastic added 2 commits January 18, 2024 08:18

APMJvmOptions global labels error

d7d6266

Merge branch 'main' of github.com:elastic/elasticsearch into telemetr…

0663cd8

…y-agent-fallback

prefer primary prefix to fallback in AffixSetting.getAsMap

0094df2

stu-elastic changed the title ~~Metrics: Agent settings prefix telemetry.agent deprecates tracing.apm.agent~~ Metrics: Agent settings prefix telemetry.agent preferred over tracing.apm.agent Jan 18, 2024

mosche approved these changes Jan 18, 2024

View reviewed changes

stu-elastic added 2 commits January 22, 2024 09:43

deprecation is not backwards compatible, removed from this PR

bb03636

update references from tracing.apm.agent. to telemetry.agent.

a3499bd

stu-elastic merged commit 3493e42 into elastic:main Jan 22, 2024

pgomulka mentioned this pull request Jan 29, 2024

New APM settings using telemetry. prefix deprecate ambiguous tracing.apm. settings. #104376

Merged

mosche mentioned this pull request Jan 30, 2024

Deprecate Telemetry / APM legacy settings in favor of the new telemetry.* settings #104908

Merged

mosche mentioned this pull request Mar 7, 2024

Introduce setting aliases to migrate settings more easily. #106061

Closed

Conversation

stu-elastic commented Jan 12, 2024

Uh oh!

elasticsearchmachine commented Jan 12, 2024

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosche Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mosche Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stu-elastic commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stu-elastic commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mosche left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mosche Jan 15, 2024 •

edited

Loading

mosche Jan 15, 2024 •

edited

Loading

stu-elastic commented Jan 17, 2024 •

edited

Loading