Skip to content

telemetry: add support for reporting_interval #2556

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

Merged
merged 3 commits into from
Nov 14, 2022

Conversation

zirain
Copy link
Member

@zirain zirain commented Nov 9, 2022

for istio/istio#41763

allow end-users config tcp_reporting_duration via Telemetry API

in v1.17 istio.stats will use native filter, the default value is 5s as kuat's suggestions: istio/istio#41763 (comment)

@zirain zirain added the release-notes-none Indicates a PR that does not require release notes. label Nov 9, 2022
@zirain zirain requested a review from a team as a code owner November 9, 2022 04:01
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 9, 2022
@nrjpoddar
Copy link
Member

Is there ay other user facing impact to switching to the native filter? Since this config option only applies to TCP metrics we need to evaluate where to add this field in the API.

@zirain
Copy link
Member Author

zirain commented Nov 9, 2022

Is there ay other user facing impact to switching to the native filter?

IMO no, but @kyessenov can confirm this.

@kyessenov
Copy link
Contributor

  1. Native vs wasm - the difference is where the timer is installed. In Wasm, it's one big timer per worker, in native, it's per individual stream. The duration applies to both.
  2. 5s is reasonable because that's the default envoy flush interval. Anything faster won't help.
  3. gRPC stream metrics are also reported continuously for HTTP, but that's a little known feature.

@kyessenov
Copy link
Contributor

LGTM, users are asking for this, and we need to stop EnvoyFilter method.
Can you rename it to "reporting_interval"? We may use this for long duration HTTP streams.

@zirain zirain changed the title telemetry: add support for tcp_reporting_duration telemetry: add support for reporting_interval Nov 10, 2022
@zirain
Copy link
Member Author

zirain commented Nov 10, 2022

LGTM, users are asking for this, and we need to stop EnvoyFilter method. Can you rename it to "reporting_interval"? We may use this for long duration HTTP streams.

done

Comment on lines +401 to +402
// Optional. Reporting interval allows configuration of the time between calls out to for metrics reporting.
// This currently only supports TCP metrics but we may use this for long duration HTTP streams in the future.
Copy link

Choose a reason for hiding this comment

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

Maybe:

Suggested change
// Optional. Reporting interval allows configuration of the time between calls out to for metrics reporting.
// This currently only supports TCP metrics but we may use this for long duration HTTP streams in the future.
// Optional. Reporting interval allows configuration of the time between calls for metrics reporting.
// This currently supports TCP metrics only, but we may use this for long duration HTTP streams in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes-none Indicates a PR that does not require release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants