Skip to content
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

Set default service.environment #4468

Closed
sorenlouv opened this issue Nov 30, 2020 · 20 comments
Closed

Set default service.environment #4468

sorenlouv opened this issue Nov 30, 2020 · 20 comments

Comments

@sorenlouv
Copy link
Member

sorenlouv commented Nov 30, 2020

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
Copy link
Member

axw commented Dec 1, 2020

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
Copy link
Member Author

@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
Copy link
Member

axw commented Dec 8, 2020

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

sorenlouv commented Dec 8, 2020

+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: elastic/kibana#83256 (comment)

@felixbarny
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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

@sorenlouv
Copy link
Member Author

sorenlouv commented Feb 21, 2022

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?

  • 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.
  • Filtering by environment has the potential to significantly speed up ES queries if there are many environments

@axw
Copy link
Member

axw commented Feb 22, 2022

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
Copy link
Member

@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
Copy link
Member Author

@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 simitt changed the title Make service.environment required Set default service.environment Mar 7, 2022
@axw axw removed the v8.0.0 label Mar 15, 2022
@simitt simitt added this to the 8.3 milestone Mar 30, 2022
@simitt simitt removed this from the 8.3 milestone Mar 31, 2022
@simitt
Copy link
Contributor

simitt commented Mar 31, 2022

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

@simitt
Copy link
Contributor

simitt commented Nov 29, 2024

@sorenlouv @axw is this still relevant or can we close and open a new request if it becomes relevant at some point?

@axw
Copy link
Member

axw commented Dec 2, 2024

IMO it makes less sense as we move towards OTel, where data is not necessarily even service-related. I'm inclined to close it and reopen later as needed. @felixbarny WDYT?

@felixbarny
Copy link
Member

Yes, I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants