Truncate `deploymentStatus` metrics after reporting stats #4857

t-kikuc · 2024-04-03T07:57:24Z

What this PR does / why we need it:

Delete deploymentStatus metrics after reporting stats
in order to avoid the error of excess grpc message size by accumulated deploymentStatus records.

Which issue(s) this PR fixes:

Fixes #4786

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

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

t-kikuc · 2024-04-03T07:58:48Z

I will remove the wip tag after discussing the solution for the issue #4786 ~~#4857~~

codecov · 2024-04-03T08:01:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.89%. Comparing base (7b1103d) to head (d586946).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4857      +/-   ##
==========================================
- Coverage   28.90%   28.89%   -0.01%     
==========================================
  Files         317      317              
  Lines       40369    40369              
==========================================
- Hits        11668    11664       -4     
- Misses      27773    27776       +3     
- Partials      928      929       +1

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

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

khanhtc1202

Nice catch, thank you 🚀

ffjlabo · 2024-04-04T06:36:29Z

@t-kikuc
[ASK] Is it OK for the Control Plane?
Just to be sure, I think it would be nice to check whether this change has any effect on the Control Plane as well.

ffjlabo · 2024-04-04T06:38:17Z

The point is how the sent data is used on Control Plane 👀

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

t-kikuc · 2024-04-05T05:04:47Z

deployment_status is used in the grafana-dashboard.

I'm keep investigating the effect of this change.

pipecd/manifests/pipecd/grafana-dashboards/control-plane/overview.json

Line 406 in 7b1103d

    
           "expr": "sum(floor(increase(deployment_status{pipecd_component=\"piped\",project=~\"$project\",status=~\"DEPLOYMENT_CANCELLED|DEPLOYMENT_SUCCESS|DEPLOYMENT_FAILURE\"}[24h])))",

t-kikuc · 2024-04-05T05:15:13Z

Regardless of this change, when a piped reboots, stats in the Control Plane are rewritten to only newer deployments after piped's reboot.
That's because (1) stats in a piped are reset when the piped restarts and (2) whole stats of a piped in the Control Plane are overwritten by statsreporter (see below).

pipecd/pkg/app/server/grpcapi/piped_api.go

Line 156 in 7b1103d

if err := a.pipedStatCache.Put(pipedID, val); err != nil {

If this matters, we need to fix the data structure/algorithm of storing stats in the Control Plane's redis.

ffjlabo

@t-kikuc
Thank you for the fix and difficult investigation 🚀

It would be nice to write the result of the investigation and make a PR for fixing document later.

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

khanhtc1202

Big step forward, thank you 🙌

t-kikuc · 2024-04-05T05:27:32Z

I'll open a PR of doc later!

* Truncate deploymentStatus metrics after reporting to avoid excess message size Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Rename func to Flush() for clarity Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Add comment of what's included in statsreporter's body Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Fix indent in the comment Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Copy change of metrics.go to pipedv1 Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Copy change of reporter.go to pipedv1 Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> --------- Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

* BUGFIX: Added cancel method to fix context leak (#4767) Signed-off-by: fazledyn-or <ataf@openrefactory.com> * Define piped pluggin api (#4815) Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Update BuldPlan API for piped pluggin (#4821) Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Relocate plugin proto (#4826) Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Update controller to use new planner logic (#4825) * Update controller to use new planner logic Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Update proto path Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Fix typo Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Fix typo Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Update planner logic to call proto instead of self executing Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> --------- Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> * Update plugin proto for ExecutorService and add piped pluginservice (#4834) * Add plugin planner for k8s (#4819) * [WIP] Add planner Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Not to use out.Version Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Use last_successful_commit_hash and last_successful_config_file_name Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Use in.WorkingDir Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Use in.PipedConfig Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Create git client Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Create secret encryptor Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add startup server implementation Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Fix for relocation of proto api Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add roughly implementation for planner plugin Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Rename pkg name Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add licence Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Comment out for the testing code Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Truncate `deploymentStatus` metrics after reporting stats (#4857) * Truncate deploymentStatus metrics after reporting to avoid excess message size Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Rename func to Flush() for clarity Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Add comment of what's included in statsreporter's body Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Fix indent in the comment Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Copy change of metrics.go to pipedv1 Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Copy change of reporter.go to pipedv1 Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> --------- Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> --------- Signed-off-by: fazledyn-or <ataf@openrefactory.com> Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Co-authored-by: Ataf Fazledin Ahamed <ataf@openrefactory.com> Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com> Co-authored-by: Yoshiki Fujikane <40124947+ffjlabo@users.noreply.github.com>

* Truncate deploymentStatus metrics after reporting to avoid excess message size Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Rename func to Flush() for clarity Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Add comment of what's included in statsreporter's body Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Fix indent in the comment Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Copy change of metrics.go to pipedv1 Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Copy change of reporter.go to pipedv1 Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> --------- Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Truncate deploymentStatus metrics after reporting to avoid excess mes…

71d0a5b

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

t-kikuc requested review from nghialv, khanhtc1202, kentakozuka and ffjlabo as code owners April 3, 2024 07:57

github-actions bot added the area/go label Apr 3, 2024

t-kikuc added do-not-merge/wip and removed area/go labels Apr 3, 2024

t-kikuc mentioned this pull request Apr 4, 2024

Piped restart due to failed when report stats to controlplane #4786

Closed

t-kikuc added 2 commits April 4, 2024 12:35

Rename func to Flush() for clarity

9aaa585

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

Add comment of what's included in statsreporter's body

ea1b555

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

github-actions bot added the area/go label Apr 4, 2024

Fix indent in the comment

b69e3db

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

t-kikuc removed the do-not-merge/wip label Apr 4, 2024

khanhtc1202 previously approved these changes Apr 4, 2024

View reviewed changes

Copy change of metrics.go to pipedv1

de79485

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

t-kikuc dismissed khanhtc1202’s stale review via de79485 April 5, 2024 04:43

ffjlabo previously approved these changes Apr 5, 2024

View reviewed changes

Copy change of reporter.go to pipedv1

d586946

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

t-kikuc dismissed ffjlabo’s stale review via d586946 April 5, 2024 05:21

ffjlabo approved these changes Apr 5, 2024

View reviewed changes

khanhtc1202 approved these changes Apr 5, 2024

View reviewed changes

t-kikuc merged commit c853313 into master Apr 5, 2024
13 of 14 checks passed

t-kikuc deleted the delete-old-deployment-status-metrics branch April 5, 2024 05:27

github-actions bot mentioned this pull request Apr 8, 2024

Release v0.46.1 #4859

Merged

t-kikuc mentioned this pull request Apr 8, 2024

Cherry-pick #4767 #4815 #4821 #4826 #4825 #4834 #4819 #4857 #4860

Merged

This was referenced Apr 9, 2024

Release v0.47.0 #4863

Merged

Add a caution comment of scrape_interval #4869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Truncate `deploymentStatus` metrics after reporting stats #4857

Truncate `deploymentStatus` metrics after reporting stats #4857

t-kikuc commented Apr 3, 2024

t-kikuc commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading

khanhtc1202 left a comment

ffjlabo commented Apr 4, 2024

ffjlabo commented Apr 4, 2024

t-kikuc commented Apr 5, 2024

t-kikuc commented Apr 5, 2024

ffjlabo left a comment

khanhtc1202 left a comment

t-kikuc commented Apr 5, 2024

Truncate deploymentStatus metrics after reporting stats #4857

Truncate deploymentStatus metrics after reporting stats #4857

Conversation

t-kikuc commented Apr 3, 2024

t-kikuc commented Apr 3, 2024 • edited Loading

codecov bot commented Apr 3, 2024 • edited Loading

Codecov Report

khanhtc1202 left a comment

Choose a reason for hiding this comment

ffjlabo commented Apr 4, 2024

ffjlabo commented Apr 4, 2024

t-kikuc commented Apr 5, 2024

t-kikuc commented Apr 5, 2024

ffjlabo left a comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

t-kikuc commented Apr 5, 2024

Truncate `deploymentStatus` metrics after reporting stats #4857

Truncate `deploymentStatus` metrics after reporting stats #4857

t-kikuc commented Apr 3, 2024 •

edited

Loading

codecov bot commented Apr 3, 2024 •

edited

Loading