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 a caution comment of scrape_interval #4869

Merged
merged 3 commits into from
Apr 18, 2024
Merged

Conversation

t-kikuc
Copy link
Member

@t-kikuc t-kikuc commented Apr 11, 2024

What this PR does / why we need it:

Add a caution comment against changing scrape_interval of Prometheus to be larger than 1m.

Which issue(s) this PR fixes:

Related to #4857

Does this PR introduce a user-facing change?: no

  • How are users affected by this change: no
  • Is this breaking change: no
  • How to migrate (if breaking change): no

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@@ -200,6 +200,8 @@ data:
replacement: $1

- job_name: pipecd-ops
# This scrape_interval must not be larger than 1m, which is the interval of piped's stats-reporting.
Copy link
Member

@khanhtc1202 khanhtc1202 Apr 11, 2024

Choose a reason for hiding this comment

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

Should mention it as not less than piped stat-reporting interval is enough. We may change the interval of that piped stat reporting later, wdyt?

Copy link
Member Author

@t-kikuc t-kikuc Apr 11, 2024

Choose a reason for hiding this comment

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

@khanhtc1202
Thanks, I agree with you.
I fixed as below:

# This scrape_interval must not be larger than the interval of piped's stats-reporting.

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.21%. Comparing base (4436527) to head (280e0eb).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4869      +/-   ##
==========================================
+ Coverage   28.91%   29.21%   +0.29%     
==========================================
  Files         317      317              
  Lines       40369    40564     +195     
==========================================
+ Hits        11674    11849     +175     
- Misses      27767    27778      +11     
- Partials      928      937       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@t-kikuc t-kikuc enabled auto-merge (squash) April 11, 2024 01:54
@@ -200,6 +200,8 @@ data:
replacement: $1

- job_name: pipecd-ops
# This scrape_interval must not be larger than the interval of piped's stats-reporting.
# ref: https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/statsreporter/reporter.go#L53
Copy link
Member

Choose a reason for hiding this comment

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

[IMO]
The link leads to the code for the latest master. So, there is a possibility of changing the position if someone change the file (e.g. delete or add some lines)

So how about adding the interval description to the docs and referring to it on this line?

Copy link
Member

@khanhtc1202 khanhtc1202 Apr 18, 2024

Choose a reason for hiding this comment

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

@t-kikuc @ffjlabo Let's wrap up to finish this. I think explain all interval in docs is a good idea but we should have this inline comment also. So why not just leave this comment here and address docs explain later, we can even make a blog for the whole metric system of pipecd later (as I told @t-kikuc before). If you worry about the master branch reference, let fix it as snapshot (commit hash) branch reference

Copy link
Member

Choose a reason for hiding this comment

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

Sure, so just finish the PR and later make docs PR 👍

Copy link
Member

Choose a reason for hiding this comment

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

If you worry about the master branch reference, let fix it as snapshot (commit hash) branch reference

@t-kikuc plz change only it 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

@khanhtc1202 @ffjlabo
thanks, I fixed using a73d091 (v0.47.0 tag's hash)

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

Thank you 🚀 🌔

Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

🚀

@t-kikuc t-kikuc merged commit a4b49a9 into master Apr 18, 2024
14 checks passed
@t-kikuc t-kikuc deleted the caution-prometheus-interval branch April 18, 2024 08:35
@khanhtc1202 khanhtc1202 mentioned this pull request Apr 18, 2024
@github-actions github-actions bot mentioned this pull request Apr 22, 2024
khanhtc1202 pushed a commit that referenced this pull request Apr 22, 2024
* Add caution comment of scrape_interval

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Remove concrete value

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Fix to commit hash of v0.47.0 to fix the ref position

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
khanhtc1202 pushed a commit that referenced this pull request Apr 22, 2024
* Add caution comment of scrape_interval

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Remove concrete value

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Fix to commit hash of v0.47.0 to fix the ref position

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
khanhtc1202 added a commit that referenced this pull request Apr 22, 2024
#4883 #4887 #4885 #4886 #4884 #4880 (#4890)

* Update contributors list (#4866)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Use valid semver in run/pipecd command (#4851)

* Use valid semver in run/pipecd command

Signed-off-by: David <19214156+dgannon991@users.noreply.github.com>

* Swapped to simpler local version

Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>

* Include a comment explaining why we hard coded

Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>

---------

Signed-off-by: David <19214156+dgannon991@users.noreply.github.com>
Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Support Canary & Blue/Green for ECS by `pipectl init` (#4801)

* Add expected YAML for kustomize

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Add Kustomize pattern for pipectl init

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Add Helm pattern for pipectl init

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Add options for Helm

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* fix typo

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Add ECS canary for pipectl init

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Add pipectl init status

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* add init cmd to pipectl doc

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* add pipectl init explanation

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* fi pipectl init status

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Embed the simple AnalysisStage in pipeline by default

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Merge fix origin/master into pipectl-init-ecs-canary

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* fix prompt message and default value

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Fix output YAML structure by generic structs

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Support Blue/Green for ECS by pipectl init

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* omitempty Percentage

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Changed default values for simpler config

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Rename funcs to avoid name conflicts with other platforms

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* [doc] Fix typo x2 in DeploymentChain (#4872)

* Fix typo: archive->achieve

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Fix type: deployment->development

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Fix typos of 'achieve' and 'under development' in older docs

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* docs: added install method (#4875)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Fix unable to use SecretEncryption and Attachment features at the same time (#4855)

* Add test to mention error with go templating

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Reimplement sourceprosser logic

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* SourceProssesor without processor should be marked as error

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Add test

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Enable perform template processing in chain

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Fix typo

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

---------

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Add a caution comment of scrape_interval (#4869)

* Add caution comment of scrape_interval

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Remove concrete value

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* Fix to commit hash of v0.47.0 to fix the ref position

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

---------

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Rewrite pipectl installation docs (#4877)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Fix typo (#4878)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Set initial-branch on git init (#4882)

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Update contributors list (#4883)

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Bump golang.org/x/net from 0.17.0 to 0.23.0 (#4887)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Bump golang.org/x/net in /tool/actions-plan-preview (#4885)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Bump golang.org/x/net from 0.17.0 to 0.23.0 in /tool/actions-gh-release (#4886)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Update default versions of kubectl, kustomize and helm in configuration-reference.md (#4884)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

* Add Homebrew Formula (#4880)

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>

---------

Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com>
Signed-off-by: David <19214156+dgannon991@users.noreply.github.com>
Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
Co-authored-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
Co-authored-by: Tetsuya Kikuchi <97105818+t-kikuc@users.noreply.github.com>
Co-authored-by: YuyaKoda <29038315+ponkio-o@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Shinichi Nishimura <nshmura.s@gmail.com>
Co-authored-by: Shohei Ueda <30958501+peaceiris@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request May 13, 2024
@github-actions github-actions bot mentioned this pull request Jun 12, 2024
@github-actions github-actions bot mentioned this pull request Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants