elastic / apm-server

https://www.elastic.co/guide/en/apm/guide/current/index.html
Other
1.22k stars 523 forks source link

Set default `service.environment` #4468

Open sorenlouv opened 3 years ago

sorenlouv commented 3 years ago

Related

APM UI is planning to remove the "All environments" dropdown option, since mixing data between environments causes more harm than good. We are also going to remove the option "No environment defined". Lastly, we will default to showing services from "production". This default will be configurable and there will still be a dropdown in the UI to change environment

These changes means that services without environments won't show up any more. To solve this I suggest that service.environment is made required. Either by requiring agents to specify an environment or by filling it out automatically if empty. In this case I suggest defaulting to "production".

axw commented 3 years ago

Either by requiring agents to specify an environment or by filling it out automatically if empty. In this case I suggest defaulting to "production".

I think setting it in the server would be best. I'd like to avoid breaking compatibility with agents in 8.0 if possible, to minimise the pain of upgrading.

sorenlouv commented 3 years ago

@axw that sounds good to me 👍 This would mean that the UI can expect environment to always have a value from 8.0, right?

axw commented 3 years ago

This would mean that the UI can expect environment to always have a value from 8.0, right?

@sqren I think at the latest, yes. @AlexanderWert mentioned that there was some ongoing discussion about this, about it possibly breaking central config. I'm not sure where that discussion is happening, or if it has been resolved...

felixbarny commented 3 years ago

This would mean that the UI can expect environment to always have a value from 8.0, right?

A notable exception would be if users have an older version of APM Server with a newer version of Kibana. Although, based on telemetry, most of users update APM Server and Kibana in tandem. Also, ES might contain data from older versions.

Introducing new required fields later on is painful. Any chance you can still treat the service.environment as optional?

sorenlouv commented 3 years ago

A notable exception would be if users have an older version of APM Server with a newer version of Kibana

I don't think we officially support running two different majors of the stack (7.11 and 8.0 for instance).

Any chance you can still treat the service.environment as optional

That's what we are doing currently and would like to move away from. So yes, we can keep doing it but I think we can provide a better user experience, and remove some technical debt by simplifying the environment selector to only concrete values.

sorenlouv commented 3 years ago

Introducing new required fields later on is painful.

Btw. if we add a migration script that automatically fills out service.environment users shouldn't feel the pain.

felixbarny commented 3 years ago

I don't think we officially support running two different majors of the stack (7.11 and 8.0 for instance).

How does the process look like when doing a rolling upgrade from 7.last to 8.0? Can there be a situation where Kibana is already updated but APM Server is not or the migration script has not been executed yet?

I think we can provide a better user experience [...] by simplifying the environment selector to only concrete values

+1 on that. But could we use the missing value feature of the terms agg to group all environments without a value together?

remove some technical debt

IMHO, the first priority should be the user experience and ensuring a smooth upgrade path from 7.last to 8.x. If we can also get rid of some tech debt that would be great for sure 🙂

sorenlouv commented 3 years ago

+1 on that. But could we use the missing value feature of the terms agg to group all environments without a value together?

Funny you should mention it - we have a conversation about avoiding missing: https://github.com/elastic/kibana/issues/83256#issuecomment-740012352

felixbarny commented 3 years ago

I think it makes a lot of sense to populate those fields in the agents and/or the server. But I'm a bit skeptical whether relying on these fields to be present in the UI is a good thing. I see that it would make things easier in the UI. Although, I don't have a good feeling of how painful it is and how much of a maintenance burden it is.

But breaking if some of these quasi-required fields are not present does feel a bit like a violation of the robustness principle.

Be conservative in what you send, be liberal in what you accept

sorenlouv commented 3 years ago

Okay, that makes sense. Let's start with auto-populating it on the server, but the ui will still treat service.environment as optional and keep using missing to surface data without an environment.

sorenlouv commented 3 years ago

Closing this as we've decided to keep service.environment as optional but aim to set default values (either in agent or server) so the user is less likely to see "No environment defined" option in the ui.

dgieselaar commented 3 years ago

Fwiw, an empty string would also allow us to avoid using the missing option in a terms aggregation.

sorenlouv commented 2 years ago

After discussing this with @simitt we decided that the APM Server should default to "development" if no other environment is set. This will enable the UI to filter by "development" environment by default (configurable).

Why this change?

axw commented 2 years ago

Currently the UI defaults to showing data across all environments. This creates a bad initial experience since mixing environments creates unexpected results. ML jobs are restricted to specific environments so won't show up in this case.

@sqren are we still considering the option of not merging services from multiple environments, but instead splitting them and listing the environment next to the service name?

dgieselaar commented 2 years ago

@axw one concern there is that it will create new UX problems: how do we deal with eg dozens of environments for a single service, should we also separate nodes on the service map by environment, and dependencies, etc.

sorenlouv commented 2 years ago

@sqren are we still considering the option of not merging services from multiple environments, but instead splitting them and listing the environment next to the service name?

For the reasons @dgieselaar mentioned we are not going to do that just yet. Instead we are trying to guide the user towards filtering by a specific environment by default.

simitt commented 2 years ago

Removing from 8.3 planning according to https://github.com/elastic/apm-dev/issues/689#issuecomment-1083557564