Introduce a configurable default service environment #4861

axw · 2021-02-22T12:31:51Z

Motivation/summary

Introduce a configurable default service environment. This is recorded in events which have no service environment defined, and is used in agent central config when no environment is specified. There is no default default, so there is no change in behaviour unless the configuration is set.

Checklist

Update CHANGELOG.asciidoc
Documentation has been updated

How to test these changes

Run apm-server configured with apm-server.default_service_environment: abc123
Send some transactions with environment set (to something else); the environment should be kept intact, and not overridden
Send some transactions with environment unset; the default environment should be set
Enable transaction aggregation, ensure all metrics have an environment associated
Enable central config, ensure that an agent with no environment set picks up config for the environment "abc123"

Related issues

Closes #4664.

apmmachine · 2021-02-22T13:14:39Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #4861 updated
Start Time: 2021-02-26T00:20:36.059+0000
Duration: 58 min 23 sec
Commit: 19b5477

Test stats 🧪

Test	Results
Failed	0
Passed	4738
Skipped	118
Total	4856

Trends 🧪

codecov-io · 2021-02-23T03:56:14Z

Codecov Report

Merging #4861 (1f2445e) into master (24b6886) will decrease coverage by 0.05%.
The diff coverage is 62.85%.

@@            Coverage Diff             @@
##           master    #4861      +/-   ##
==========================================
- Coverage   76.77%   76.71%   -0.06%     
==========================================
  Files         166      167       +1     
  Lines       10230    10265      +35     
==========================================
+ Hits         7854     7875      +21     
- Misses       2376     2390      +14

Impacted Files	Coverage Δ
beater/config/config.go	`68.85% <ø> (ø)`
x-pack/apm-server/main.go	`0.00% <ø> (ø)`
beater/beater.go	`67.93% <27.77%> (-2.66%)`	⬇️
beater/api/config/agent/handler.go	`95.04% <100.00%> (+0.08%)`	⬆️
beater/api/mux.go	`91.83% <100.00%> (ø)`
model/modelprocessor/environment.go	`100.00% <100.00%> (ø)`

jalvz · 2021-02-25T09:44:32Z

model/modelprocessor/environment.go

+		case *model.Error:
+			if t.Metadata.Service.Environment == "" {
+				t.Metadata.Service.Environment = s.DefaultServiceEnvironment
+			}


should be exhaustive here? i prefer prefer that something fails loudly if eg. we add a new event type

Yeah, good point - it should be and it's not. I forgot to cover profiles.

I could make this panic if it sees a type it doesn't know about, but I'd rather not have this feature cause such a catastrophic failure in case test coverage isn't quite adequate (which evidently it's not).

I think ideally we wouldn't be dealing with a "Transformable" interface here at all, but instead pass around the concrete model.Batch struct type. Then the processor can just iterate over the bits it cares about. We could have tests for processors that check the Batch type hasn't changed, if necessary.

I'd like to defer that, and just add Profile in for now. WDYT?

sounds good, thanks (create an issue, maybe?)

Opened #4877

jalvz · 2021-02-25T09:44:44Z

apmpackage/apm/0.1.0/manifest.yml

@@ -62,6 +62,12 @@ policy_templates:
        required: false
        description: Enter as <Id>:<API Key>
        show_user: true
+      - name: default_service_environment


not sure about this, shouldn't we use the namespace instead?

I'd prefer to keep them separate for now, so users have the flexibility to have two dimensions:

a namespace is a collection of APM services, e.g. split by business applications, business unit, etc.

typical environments for those services: production, testing, development

ah ok, that was news to me 👍

Please also update the README accordingly, it refers to using namespace for environment.

Just to be clear, setting namespace to environment is still a reasonable choice. Perhaps we should just soften the language a little, to point it out as an option but not necessarily state that it is recommended.

yeah, that's what I meant, the additional use case for namespace tha you explained above, should be added to the README.

simitt · 2021-02-25T12:08:07Z

apmpackage/apm/0.1.0/manifest.yml

@@ -62,6 +62,12 @@ policy_templates:
        required: false
        description: Enter as <Id>:<API Key>
        show_user: true
+      - name: default_service_environment


Please also update the README accordingly, it refers to using namespace for environment.

simitt · 2021-02-25T12:37:43Z

Apologies - I just approved, but didn't see your ongoing conversation.

jalvz · 2021-02-25T14:56:03Z

One more question, sorry for delay: this intended for 7.13, I suppose?

axw · 2021-02-26T00:04:10Z

@jalvz yes, now labelled accordingly

jalvz · 2021-02-26T08:51:47Z

@jalvz yes, now labelled accordingly

I was going to say that in that case better hold onto merging to allow for package changes for 7.12 😬

axw · 2021-02-27T02:56:40Z

@jalvz I suppose if that's needed we should be working off the 7.12 branch instead. We'll eventually version the package with the stack right? So bump the version on master/7.x to 0.2.0 and have 0.1.0 on 7.12?

* Add transform.Processor interface * beater: add default service environment * model/modelprocessor: update PprofProfile too * apmpackage: update README about namespace # Conflicts: # apmpackage/apm/0.1.0/_dev/docs/README.template.md # apmpackage/apm/0.1.0/docs/README.md # apmpackage/apm/0.1.0/manifest.yml # changelogs/head.asciidoc

* Add transform.Processor interface * beater: add default service environment * model/modelprocessor: update PprofProfile too * apmpackage: update README about namespace (cherry picked from commit 992699d) # Conflicts: # apmpackage/apm/0.1.0/manifest.yml # apmpackage/apm/0.2.0/README.template.md # apmpackage/apm/0.2.0/docs/README.md # beater/api/config/agent/handler_test.go # beater/beater.go # beater/config/config.go # changelogs/head.asciidoc # model/modelprocessor/environment.go # model/modelprocessor/environment_test.go # transform/transform.go # x-pack/apm-server/main.go

sorenlouv · 2022-02-10T12:42:25Z

@axw Are there any plans to enable the default service environment by default? In APM UI we are planning to remove the "all" option as well as the "no environment" option. If we do that and users have data without environment it won't show up anymore.

axw · 2022-02-11T02:41:44Z

@sqren no plans at the moment. I recall we discussed this (taking away "all" and "none") in the past, and decided that it wasn't doable at the time. I think maybe because of backwards compatibility? What are we going to do for existing data?

sorenlouv · 2022-02-11T10:44:17Z

I recall we discussed this (taking away "all" and "none") in the past, and decided that it wasn't doable at the time. I think maybe because of backwards compatibility? What are we going to do for existing data?

Yes, removing them now can be seen as a breaking change. Which is why I'd like to add a default environment asap so we can do it in ~18 months time (or whatever our data guarantees for APM data is).

We could add a migration script that updates documents to ensure they have an environment. But it requires a re-index of all data so I'm not sure how feasible it is.

simitt · 2022-02-11T10:49:15Z

@sqren can you create an issue for this so we can continue the discussion, including what the default env should be set to? I am not certain setting a default on the server would be more meaningful than having a none or all in the UI.

sorenlouv · 2022-02-11T23:17:10Z

Should I re-open https://github.com/elastic/observability-dev/issues/1118 since it already contains all the background?

axw · 2022-02-14T01:37:30Z

@sqren I think reopen #4468, with anything that's changed since it was closed.

codecov-commenter · 2024-10-11T21:34:35Z

Codecov Report

Attention: Patch coverage is 65.78947% with 13 lines in your changes missing coverage. Please review.

Project coverage is 76.53%. Comparing base (acc9d81) to head (19b5477).

Files with missing lines	Patch %	Lines
beater/beater.go	27.77%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4861      +/-   ##
==========================================
- Coverage   76.58%   76.53%   -0.06%     
==========================================
  Files         166      167       +1     
  Lines       10118    10156      +38     
==========================================
+ Hits         7749     7773      +24     
- Misses       2369     2383      +14

Files with missing lines	Coverage Δ
beater/api/config/agent/handler.go	`95.04% <100.00%> (+0.08%)`	⬆️
beater/api/mux.go	`91.83% <100.00%> (ø)`
beater/config/config.go	`68.85% <ø> (ø)`
model/modelprocessor/environment.go	`100.00% <100.00%> (ø)`
x-pack/apm-server/main.go	`0.00% <ø> (ø)`
beater/beater.go	`67.93% <27.77%> (-2.66%)`	⬇️

axw changed the title ~~Processor interface~~ Introduce a configurable default service environment Feb 22, 2021

axw force-pushed the processor-interface branch from 7f8c1c6 to 7cb3eab Compare February 23, 2021 03:37

axw force-pushed the processor-interface branch from 7cb3eab to 8a37add Compare February 23, 2021 05:08

axw added 2 commits February 23, 2021 13:08

Add transform.Processor interface

e14d176

beater: add default service environment

5d10a73

axw force-pushed the processor-interface branch from 8a37add to 5d10a73 Compare February 23, 2021 05:09

Merge branch 'master' into processor-interface

ac5f265

axw marked this pull request as ready for review February 25, 2021 02:50

Merge branch 'master' into processor-interface

1f2445e

axw requested a review from a team February 25, 2021 05:02

Merge branch 'master' into processor-interface

ac35cb2

jalvz reviewed Feb 25, 2021

View reviewed changes

simitt approved these changes Feb 25, 2021

View reviewed changes

model/modelprocessor: update PprofProfile too

e5cf4ff

axw added test-plan v7.13.0 labels Feb 26, 2021

axw mentioned this pull request Feb 26, 2021

model: processors should have a safe means of exhaustively identifying model types #4877

Closed

axw added 2 commits February 26, 2021 08:16

Merge branch 'master' into processor-interface

6c7f119

apmpackage: update README about namespace

19b5477

axw merged commit 992699d into elastic:master Feb 26, 2021

axw deleted the processor-interface branch February 26, 2021 01:20

axw removed the test-plan label Mar 8, 2021

axw mentioned this pull request Mar 25, 2021

[7.x] Introduce a configurable default service environment (#4861) #4996

Merged

jalvz added the test-plan label Apr 27, 2021

mergify bot mentioned this pull request Apr 27, 2021

[7.13] Introduce a configurable default service environment (backport #4861) #5143

Closed

jalvz added the test-plan-ok label May 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a configurable default service environment #4861

Introduce a configurable default service environment #4861

axw commented Feb 22, 2021 •

edited

Loading

apmmachine commented Feb 22, 2021 •

edited

Loading

Build stats

Test stats 🧪

Trends 🧪

codecov-io commented Feb 23, 2021 •

edited

Loading

jalvz Feb 25, 2021

axw Feb 25, 2021 •

edited

Loading

jalvz Feb 25, 2021

axw Feb 26, 2021

jalvz Feb 25, 2021

axw Feb 25, 2021

jalvz Feb 25, 2021

simitt Feb 25, 2021

axw Feb 25, 2021

simitt Feb 25, 2021

axw Feb 26, 2021

simitt Feb 25, 2021

simitt commented Feb 25, 2021

jalvz commented Feb 25, 2021

axw commented Feb 26, 2021

jalvz commented Feb 26, 2021

axw commented Feb 27, 2021

sorenlouv commented Feb 10, 2022

axw commented Feb 11, 2022

sorenlouv commented Feb 11, 2022

simitt commented Feb 11, 2022

sorenlouv commented Feb 11, 2022

axw commented Feb 14, 2022

codecov-commenter commented Oct 11, 2024 •

edited

Loading

Introduce a configurable default service environment #4861

Introduce a configurable default service environment #4861

Conversation

axw commented Feb 22, 2021 • edited Loading

Motivation/summary

Checklist

How to test these changes

Related issues

apmmachine commented Feb 22, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

codecov-io commented Feb 23, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

axw Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simitt commented Feb 25, 2021

jalvz commented Feb 25, 2021

axw commented Feb 26, 2021

jalvz commented Feb 26, 2021

axw commented Feb 27, 2021

sorenlouv commented Feb 10, 2022

axw commented Feb 11, 2022

sorenlouv commented Feb 11, 2022

simitt commented Feb 11, 2022

sorenlouv commented Feb 11, 2022

axw commented Feb 14, 2022

codecov-commenter commented Oct 11, 2024 • edited Loading

Codecov Report

axw commented Feb 22, 2021 •

edited

Loading

apmmachine commented Feb 22, 2021 •

edited

Loading

codecov-io commented Feb 23, 2021 •

edited

Loading

axw Feb 25, 2021 •

edited

Loading

codecov-commenter commented Oct 11, 2024 •

edited

Loading