Skip to content

Stats: Add iteration and sink predicates for histograms#19166

Merged
jmarantz merged 41 commits intoenvoyproxy:mainfrom
pradeepcrao:histograms
Jan 10, 2023
Merged

Stats: Add iteration and sink predicates for histograms#19166
jmarantz merged 41 commits intoenvoyproxy:mainfrom
pradeepcrao:histograms

Conversation

@pradeepcrao
Copy link
Copy Markdown
Contributor

@pradeepcrao pradeepcrao commented Dec 2, 2021

Commit Message:
Additional Description:
This is a followup to #18805 that adds the ability to filter histograms to be flushed to sink. Note that this only affects which histograms are flushed and does not change which histograms are merged during every flush operation.

Risk Level: Low
Testing: Added Tests
Docs Changes: N/A
Release Notes: included.
Platform Specific Features: N/A

Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
@pradeepcrao pradeepcrao changed the title Histograms Stats: Add iteration and sink predicates for histograms Dec 2, 2021
@pradeepcrao
Copy link
Copy Markdown
Contributor Author

/assign jmarantz

@pradeepcrao
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19166 (comment) was created by @pradeepcrao.

see: more, trace.

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

looks great modulo some minor nits and one concern about histogram memory growth.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Dec 5, 2021

/wait

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 4, 2022

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 4, 2022
Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
@jmarantz jmarantz added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Jan 4, 2022
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

this looks great! Just a few nits.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Jan 5, 2022

/wait

also need to fix format.

jmarantz
jmarantz previously approved these changes Nov 10, 2022
@jmarantz
Copy link
Copy Markdown
Contributor

@ggreenway will you be able to look at this?

Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

LGTM

/wait-any

// Android.
FALSE_RUNTIME_GUARD(envoy_reloadable_features_always_use_v6);
// TODO(pradeepcrao) reset this to true after 2 releases (1.27)
FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_include_histograms);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is a FALSE_RUNTIME_GUARD instead of a RUNTIME_GUARD?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This disables filtering the histograms by default, so envoy users using sink predicates will not experience histograms silently not being flushed to sinks.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Jan 4, 2023

@pradeepcrao were you going to pick this up again or should this remain closed?

@pradeepcrao
Copy link
Copy Markdown
Contributor Author

I missed Greg's comment. Will address this now.

@pradeepcrao pradeepcrao reopened this Jan 4, 2023
Signed-off-by: pcrao <pcrao@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #19166 was synchronize by pradeepcrao.

see: more, trace.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Jan 6, 2023

CI is failing. Also is the release prediction (1.27) still accurate?

/wait

@pradeepcrao
Copy link
Copy Markdown
Contributor Author

CI is failing. Also is the release prediction (1.27) still accurate?

/wait

Qualifying the fix for the CI failure.

Given that anyone who has implemented SinkPredicates will get a build failure that they need to react to (override includeHistogram), roughly 2 releases (1.25 and 1.26) should be sufficient time to enable the behaviour by default. What do you think?

Signed-off-by: pcrao <pcrao@google.com>
Signed-off-by: pcrao <pcrao@google.com>
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

@yurykats @stevenzzzz FYI

This still lgtm :)

@pradeepcrao
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19166 (comment) was created by @pradeepcrao.

see: more, trace.

Signed-off-by: pcrao <pcrao@google.com>
@jmarantz jmarantz merged commit 8c1de04 into envoyproxy:main Jan 10, 2023
VishalDamgude pushed a commit to freshworks-oss/envoy that referenced this pull request Feb 1, 2023
…9166)

Commit Message:
Additional Description:
This is a followup to envoyproxy#18805 that adds the ability to filter histograms to be flushed to sink. Note that this only affects which histograms are flushed and does not change which histograms are merged during every flush operation.

Risk Level: Low
Testing: Added Tests
Docs Changes: N/A
Release Notes: included.
Platform Specific Features: N/A

Signed-off-by: Pradeep Rao <pcrao@google.com>
Signed-off-by: VishalDamgude <vishal.damgude@freshworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no stalebot Disables stalebot from closing an issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants