Add option to restore high cardinality for metrics for HTTP server #7429

ItalyPaleAle · 2024-01-23T00:26:36Z

This is configured using the Configuration CRD. High cardinality is enabled by default in Dapr 1.13, but shows a warning. The default behavior will be changed in 1.14.

Fixes #7295

This is configured using the Configuration CRD. High cardinality is enabled by default in Dapr 1.13, but shows a warning. The default behavior will be changed in 1.14. Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

codecov · 2024-01-23T00:39:03Z

Codecov Report

Attention: 59 lines in your changes are missing coverage. Please review.

Comparison is base (afe79bc) 62.19% compared to head (5d5eaaf) 62.11%.

Files	Patch %	Lines
pkg/diagnostics/http_monitoring.go	31.42%	41 Missing and 7 partials ⚠️
pkg/config/configuration.go	66.66%	2 Missing and 2 partials ⚠️
pkg/runtime/config.go	0.00%	3 Missing ⚠️
pkg/diagnostics/http_monitoring_legacy.go	94.11%	1 Missing and 1 partial ⚠️
pkg/channel/http/http_channel.go	75.00%	1 Missing ⚠️
pkg/diagnostics/metrics.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7429      +/-   ##
==========================================
- Coverage   62.19%   62.11%   -0.08%     
==========================================
  Files         244      245       +1     
  Lines       22297    22346      +49     
==========================================
+ Hits        13867    13880      +13     
- Misses       7281     7306      +25     
- Partials     1149     1160      +11

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

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

msfussell · 2024-01-23T05:14:10Z

Before merging this issue, please create a docs PR to update the following

…cs-cardinality

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

mukundansundar · 2024-01-24T12:59:36Z

charts/dapr/crds/configuration.yaml

+                      the HTTP server
+                    properties:
+                      increasedCardinality:
+                        description: 'If false, metrics for the HTTP server are collected


nitpick
If true,

mukundansundar · 2024-01-24T13:00:53Z

charts/dapr/crds/configuration.yaml

+                      the HTTP server
+                    properties:
+                      increasedCardinality:
+                        description: 'If false, metrics for the HTTP server are collected


same nitpick

mukundansundar · 2024-01-24T13:05:12Z

/ok-to-test

mukundansundar · 2024-01-24T13:05:20Z

/test-version-skew

mukundansundar · 2024-01-24T13:14:15Z

@ItalyPaleAle Please remove the patch file also also https://github.com/dapr/dapr/blob/master/.github/scripts/version-skew-test-patches/e2e/release-1.12/dapr-sidecar-master/0003-Added-patch-for-updated-http_metrics-test-in-1.12.patch

JoshVanL

Some comments from me :)

JoshVanL · 2024-01-24T13:01:04Z

pkg/apis/configuration/v1alpha1/types.go

+type MetricHTTP struct {
+	// If false, metrics for the HTTP server are collected with increased cardinality.
+	// The default is true in Dapr 1.13, but will be changed to false in 1.14+
+	// TODO @ItalyPaleAle: Metrics cardinality


Intended to be committed?

Yes. This is so I can search "TODO @ItalyPaleAle" in the codebase :)

I changed the text to be more explicit

pkg/config/configuration.go

pkg/diagnostics/http_monitoring.go

tests/integration/suite/daprd/metrics/helpers.go

JoshVanL · 2024-01-24T13:31:21Z

tests/integration/suite/daprd/metrics/helpers.go

+type base struct {
+	daprd      *procdaprd.Daprd
+	httpClient *http.Client
+	grpcClient runtimev1pb.DaprClient
+	grpcConn   *grpc.ClientConn
+	place      *placement.Placement


I am not a fan of having a "base" suite test case as it obfuscates what is being setup/run to the reader. We should be over explicit in tests.

Can we move the base code into its consumed test cases?

We can but then the code would be replicated multiple times and be harder to maintain

JoshVanL · 2024-01-24T13:42:15Z

tests/integration/suite/daprd/metrics/httpserver_highcardinality.go

@@ -0,0 +1,97 @@
+/*


Consider moving both http tests into a http directory. This improves test organisation.

Can't if using the base class, which is for DRY

JoshVanL · 2024-01-24T13:45:33Z

tests/integration/suite/daprd/metrics/httpserver_lowcardinality.go

+spec:
+  metrics:
+    http:
+      increasedCardinality: false


Please also have a test for when this value is nil.

Added test to pkg/config/configuration_test.go since the value will change in the future and the test is duplicate.

tests/integration/suite/daprd/metrics/httpserver_lowcardinality.go

tests/integration/suite/daprd/metrics/workflow.go

…cs-cardinality

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

…cs-cardinality Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle · 2024-01-25T18:22:29Z

/test-version-skew

ItalyPaleAle · 2024-01-25T18:22:35Z

/ok-to-test

dapr-bot · 2024-01-29T04:24:37Z

Dapr Version Skew integration test (dapr-sidecar-master - 1.12.4)

🔗 Link to Action run

Commit ref: 25cfa96

✅ Version Skew tests passed

…cs-cardinality

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

tests/integration/suite/daprd/metrics/httpserver_defaultcardinality.go

…cs-cardinality Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

artursouza · 2024-01-30T20:03:05Z

I am OK to merge this PR but need the MacOS integration tests to pass.

artursouza · 2024-01-30T21:14:10Z

This PR is adding flakiness to integration tests or a bug: https://github.com/dapr/dapr/actions/runs/7716239633?pr=7429

ItalyPaleAle · 2024-01-30T22:41:46Z

This PR is adding flakiness to integration tests or a bug: https://github.com/dapr/dapr/actions/runs/7716239633?pr=7429

Those are the same errors we have in master

artursouza · 2024-01-30T23:34:50Z

Will fix flakiness in master and re-assess this PR. We need to fix both issues to release anyway.

Yaron and I decided not to block this PR due to the additional test scenarios requested.

ItalyPaleAle requested review from a team as code owners January 23, 2024 00:26

This comment was marked as outdated.

Sign in to view

ItalyPaleAle marked this pull request as draft January 23, 2024 01:26

Added ITs and some fixes

802770f

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle added 2 commits January 23, 2024 18:25

Merge branch 'master' of https://github.com/dapr/dapr into high-metri…

6eb22d5

…cs-cardinality

Fixed tests

bec4f9a

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle marked this pull request as ready for review January 23, 2024 18:44

This comment was marked as outdated.

Sign in to view

ItalyPaleAle and others added 2 commits January 23, 2024 20:15

Fixed E2E tests

8db0e0e

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Merge branch 'master' into high-metrics-cardinality

630e04d

mukundansundar reviewed Jan 24, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

JoshVanL requested changes Jan 24, 2024

View reviewed changes

ItalyPaleAle added 4 commits January 25, 2024 00:22

Merge branch 'master' of https://github.com/dapr/dapr into high-metri…

c08bc7c

…cs-cardinality

Code review

e5d2e5a

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Code review pt2

df27780

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

Merge branch 'master' of https://github.com/dapr/dapr into high-metri…

a7cebaa

…cs-cardinality Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

yaron2 and others added 4 commits January 29, 2024 07:09

Merge branch 'master' into high-metrics-cardinality

49e9a55

Merge branch 'master' of https://github.com/dapr/dapr into high-metri…

f6cbda1

…cs-cardinality

Add IT with default configuration

f722700

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

fix nits

2a025f6

yaron2 previously approved these changes Jan 29, 2024

View reviewed changes

JoshVanL previously requested changes Jan 29, 2024

View reviewed changes

tests/integration/suite/daprd/metrics/httpserver_defaultcardinality.go Show resolved Hide resolved

artursouza added 2 commits January 29, 2024 16:21

Merge branch 'master' into high-metrics-cardinality

fc1f738

Merge branch 'master' into high-metrics-cardinality

efd04b4

ItalyPaleAle added the autoupdate DaprBot will keep the Pull Request up to date with master branch label Jan 30, 2024

Merge branch 'master' of https://github.com/dapr/dapr into high-metri…

cc55688

…cs-cardinality Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

ItalyPaleAle dismissed yaron2’s stale review via cc55688 January 30, 2024 16:16

dapr-bot and others added 2 commits January 30, 2024 10:15

Merge branch 'master' into high-metrics-cardinality

c82e1af

Merge branch 'master' into high-metrics-cardinality

09b7a27

dapr-bot and others added 8 commits January 30, 2024 15:52

Merge branch 'master' into high-metrics-cardinality

65c9f72

Merge branch 'master' into high-metrics-cardinality

8c577c2

Merge branch 'master' into high-metrics-cardinality

d342bf4

Merge branch 'master' into high-metrics-cardinality

8f8e603

Merge branch 'master' into high-metrics-cardinality

5a96b94

Merge branch 'master' into high-metrics-cardinality

61701e8

Merge branch 'master' into high-metrics-cardinality

dc1bb8c

Merge branch 'master' into high-metrics-cardinality

5d5eaaf

yaron2 approved these changes Feb 6, 2024

View reviewed changes

artursouza merged commit 6b7c94f into dapr:master Feb 6, 2024
31 of 32 checks passed

JoshVanL added this to the v1.13 milestone Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to restore high cardinality for metrics for HTTP server #7429

Add option to restore high cardinality for metrics for HTTP server #7429

ItalyPaleAle commented Jan 23, 2024

This comment was marked as outdated.

codecov bot commented Jan 23, 2024 •

edited

msfussell commented Jan 23, 2024

This comment was marked as outdated.

mukundansundar Jan 24, 2024

mukundansundar Jan 24, 2024

mukundansundar commented Jan 24, 2024

mukundansundar commented Jan 24, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

mukundansundar commented Jan 24, 2024

JoshVanL left a comment

JoshVanL Jan 24, 2024

ItalyPaleAle Jan 24, 2024 •

edited

ItalyPaleAle Jan 25, 2024

JoshVanL Jan 24, 2024

ItalyPaleAle Jan 24, 2024

JoshVanL Jan 24, 2024

ItalyPaleAle Jan 24, 2024

JoshVanL Jan 24, 2024

ItalyPaleAle Jan 25, 2024

ItalyPaleAle commented Jan 25, 2024

ItalyPaleAle commented Jan 25, 2024

dapr-bot commented Jan 29, 2024 •

edited

artursouza commented Jan 30, 2024

artursouza commented Jan 30, 2024

ItalyPaleAle commented Jan 30, 2024

artursouza commented Jan 30, 2024

Add option to restore high cardinality for metrics for HTTP server #7429

Add option to restore high cardinality for metrics for HTTP server #7429

Conversation

ItalyPaleAle commented Jan 23, 2024

This comment was marked as outdated.

codecov bot commented Jan 23, 2024 • edited

Codecov Report

msfussell commented Jan 23, 2024

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukundansundar commented Jan 24, 2024

mukundansundar commented Jan 24, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

mukundansundar commented Jan 24, 2024

JoshVanL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItalyPaleAle Jan 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItalyPaleAle commented Jan 25, 2024

ItalyPaleAle commented Jan 25, 2024

dapr-bot commented Jan 29, 2024 • edited

Dapr Version Skew integration test (dapr-sidecar-master - 1.12.4)

✅ Version Skew tests passed

artursouza commented Jan 30, 2024

artursouza commented Jan 30, 2024

ItalyPaleAle commented Jan 30, 2024

artursouza commented Jan 30, 2024

codecov bot commented Jan 23, 2024 •

edited

ItalyPaleAle Jan 24, 2024 •

edited

dapr-bot commented Jan 29, 2024 •

edited