Skip to content

stats: disable global stats in config#2367

Merged
jpsim merged 1 commit intomainfrom
stats-disable-global-stats-in-config
Jun 15, 2022
Merged

stats: disable global stats in config#2367
jpsim merged 1 commit intomainfrom
stats-disable-global-stats-in-config

Conversation

@jpsim
Copy link
Contributor

@jpsim jpsim commented Jun 14, 2022

These are already being filtered out of our stats inclusion list, so this isn't a user-facing change, but this is needed to support the singleton removal work in #2129.

Splitting this out into its own PR to reduce the scope of the singleton removal change.

Risk Level: Low
Testing: None
Docs Changes: N/A, not a user-facing change
Release Notes: N/A, not a user-facing change

These are already being filtered out of our stats inclusion list, so
this isn't a user-facing change, but this is needed to support the
singleton removal work in
#2129.

Splitting this out into its own PR to reduce the scope of the singleton
removal change.

Signed-off-by: JP Simard <jp@jpsim.com>
@jpsim jpsim requested a review from goaway June 14, 2022 23:27
@jpsim jpsim marked this pull request as ready for review June 14, 2022 23:27
@jpsim jpsim requested a review from alyssawilk June 14, 2022 23:28
@alyssawilk alyssawilk self-assigned this Jun 15, 2022
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM though it might be worth explaining why (global stats not working for multi-envoy)

@jpsim
Copy link
Contributor Author

jpsim commented Jun 15, 2022

LGTM though it might be worth explaining why (global stats not working for multi-envoy)

Sure. Global stats are used to emit events when ENVOY_BUGs are logged, which then looks up the global stats handler, but with multi-envoy or even just releasing an instance of the engine as is possible with #2129, then the global stats handler is released which can lead to a crash.

Since we're not using this feature by configuration, it's easiest to just remove it as I'm doing here.

@jpsim jpsim merged commit 5349e8c into main Jun 15, 2022
@jpsim jpsim deleted the stats-disable-global-stats-in-config branch June 15, 2022 14:17
@alyssawilk
Copy link
Contributor

haha I meant in the code, but worst case folks can blame and look a the PR :-P

@jpsim
Copy link
Contributor Author

jpsim commented Jun 15, 2022

Added some more inline in the code here #2371

jpsim added a commit that referenced this pull request Jun 15, 2022
* origin/main:
  stats: disable global stats in config (#2367)
  test: got the client_integration_test moved over to the builder. (#2362)

Signed-off-by: JP Simard <jp@jpsim.com>
Augustyniak pushed a commit that referenced this pull request Jun 28, 2022
These are already being filtered out of our stats inclusion list, so
this isn't a user-facing change, but this is needed to support the
singleton removal work in
#2129.

Splitting this out into its own PR to reduce the scope of the singleton
removal change.

Co-authored-by: alyssawilk <alyssar@chromium.org>
Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants