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

Add delay in the agent between each metric push #4122

Merged
merged 5 commits into from
Apr 27, 2022

Conversation

knlambert
Copy link
Contributor

@knlambert knlambert commented Feb 16, 2022

Description

Currently:

  • By default, envoy sends metrics every 3 4 seconds to the GRPC service from the agent.
  • This delay is short, and not necessary since Ambassador cloud only stores data per minutes (last 30 minutes).

Changes:

  • Added a 30s cooldown period before sending metrics again.
  • Added some tests.
  • Added a debug header for telepresence.

To do (need some help :o) )

  • Expected version for this change ( Minor vs patch )
  • Fix build having non related linting errors

Testing

  • Tested in my cluster with a locally built version.
  • Unit tests

Checklist

  • I made sure to update CHANGELOG.md.
  • This is unlikely to impact how Ambassador performs at scale.
  • My change is adequately tested.

@knlambert knlambert requested a review from rp4rk February 16, 2022 19:49
@knlambert knlambert force-pushed the klambert/add-metrics-stream-delay branch from 22c7729 to 28730bd Compare February 17, 2022 17:26
@knlambert knlambert marked this pull request as ready for review February 17, 2022 18:02
@knlambert knlambert requested a review from kflynn February 17, 2022 18:02
@knlambert knlambert force-pushed the klambert/add-metrics-stream-delay branch from 28730bd to 548b6ab Compare February 22, 2022 13:44
@knlambert knlambert force-pushed the klambert/add-metrics-stream-delay branch from 548b6ab to 12d4fca Compare April 11, 2022 12:38
@knlambert knlambert requested review from alexgervais and removed request for rp4rk April 11, 2022 14:59
@knlambert knlambert changed the title Add delay in the agent between each metrics push Add delay in the agent between each metric push Apr 11, 2022
@knlambert knlambert requested a review from LukeShu April 11, 2022 15:22
@LukeShu
Copy link
Contributor

LukeShu commented Apr 11, 2022

Can you please add an entry to docs/releaseNotes.yml (and then run make $PWD/CHANGELOG.md) summarizing these changes?

pkg/agent/agent.go Outdated Show resolved Hide resolved
Copy link
Contributor

@LukeShu LukeShu left a comment

Choose a reason for hiding this comment

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

Some minor changes requested

  • change request: Don't use context.TODO()
  • change request: Don't use testify/suite
  • suggestion: Come up with a better name than metricsRelayDeadline

But on the whole it looks good!

@knlambert
Copy link
Contributor Author

Can you please add an entry to docs/releaseNotes.yml (and then run make $PWD/CHANGELOG.md) summarizing these changes?

Sure, under 2.3.0 ? or does it go in a 2.2.2 ?

@knlambert knlambert force-pushed the klambert/add-metrics-stream-delay branch 2 times, most recently from 36fecb9 to a271e69 Compare April 11, 2022 18:40
@knlambert
Copy link
Contributor Author

@LukeShu I have one question aswell about the metrics sink (that the agent is listening, and relaying).

Does it always push all the metrics ? Is there a risk I miss something by putting this 30s cooldown period ?

I see some differences between payloads, but only on some internal metrics which are not relevant to me.

image

@knlambert knlambert force-pushed the klambert/add-metrics-stream-delay branch 2 times, most recently from f961f9c to ea286e5 Compare April 11, 2022 19:55
@knlambert knlambert requested a review from LukeShu April 11, 2022 19:56
Alice-Lilith
Alice-Lilith previously approved these changes Apr 21, 2022
@knlambert
Copy link
Contributor Author

Rebased to fix conflict in the changelog file.

@knlambert knlambert requested a review from Alice-Lilith April 25, 2022 20:35
@knlambert knlambert force-pushed the klambert/add-metrics-stream-delay branch from 00379a0 to f74918d Compare April 25, 2022 21:00
@knlambert knlambert dismissed LukeShu’s stale review April 27, 2022 15:09

Changes made

Signed-off-by: Kévin Lambert <kevin.lambert.ca@gmail.com>
Signed-off-by: Kévin Lambert <kevin.lambert.ca@gmail.com>
Signed-off-by: Kévin Lambert <kevin.lambert.ca@gmail.com>
Signed-off-by: Kévin Lambert <kevin.lambert.ca@gmail.com>
@knlambert knlambert force-pushed the klambert/add-metrics-stream-delay branch from f74918d to 6676886 Compare April 27, 2022 15:14
docs/releaseNotes.yml Outdated Show resolved Hide resolved
@Alice-Lilith
Copy link
Member

setting DCO to pass since @knlambert is with Ambassador Labs

@Alice-Lilith Alice-Lilith merged commit 9098031 into master Apr 27, 2022
@Alice-Lilith Alice-Lilith deleted the klambert/add-metrics-stream-delay branch April 27, 2022 17:07
@Alice-Lilith Alice-Lilith mentioned this pull request Apr 27, 2022
4 tasks
@khussey
Copy link
Contributor

khussey commented May 19, 2022

@AliceProxy I guess this needs to be merged over to release/v2.3 in order to make the 2.3 release?

@Alice-Lilith
Copy link
Member

@khussey I've already cherry-picked and merged these commits into the 2.3 release branch :)

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.

4 participants