Merged metrics with consul-dataplane #1635
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
curtbushko
force-pushed
the
curtbushko/agentless-metrics-merging
branch
from
09c0e45
to
d377b69
Compare
curtbushko
force-pushed
the
curtbushko/agentless-metrics-merging
branch
3 times, most recently
from
62d1d90
to
32cb989
Compare
ishustava
reviewed
Oct 19, 2022
acceptance/tests/fixtures/bases/static-metrics-app/deployment.yaml
Outdated
Show resolved
Hide resolved
| // if data.PrometheusKeyFile == "" { | ||
| // return corev1.Container{}, fmt.Errorf("must set %q when providing prometheus TLS config", annotationPrometheusKeyFile) | ||
| // } | ||
| //} |
There was a problem hiding this comment.
are there any tests for this that we need to remove/migrate?
There was a problem hiding this comment.
The tests for this have been migrated over to consul_dataplane_test.go.
Pass metrics flags to consul-dataplane
Cleanup: remove consul-sidecar code, annotations and tests remove consul-sidecar from values.yaml and bats tests
Get acceptance tests running
curtbushko
force-pushed
the
curtbushko/agentless-metrics-merging
branch
from
6536020
to
c305e76
Compare
…-merging"" This reverts commit f1fbc3e.
wilkermichael
approved these changes
Oct 21, 2022
| env := suite.Environment() | ||
| cfg := suite.Config() | ||
| ctx := env.DefaultContext(t) | ||
| ns := ctx.KubectlOptions(t).Namespace | ||
|
|
||
| helmValues := map[string]string{ | ||
| // Remove before merging | ||
| "global.imageConsulDataplane": "curtbushko/consul-dataplane:latest", | ||
| "global.imageK8S": "curtbushko/consul-k8s-control-plane-dev:latest", |
There was a problem hiding this comment.
Don't forget to remove!
| env := suite.Environment() | ||
| cfg := suite.Config() | ||
| ctx := env.DefaultContext(t) | ||
| ns := ctx.KubectlOptions(t).Namespace | ||
|
|
||
| helmValues := map[string]string{ | ||
| "global.datacenter": "dc1", | ||
| "global.metrics.enabled": "true", | ||
| // Remove before merging |
There was a problem hiding this comment.
Don't forget to remove!
ishustava
approved these changes
Oct 22, 2022
| -log-json={{ $root.Values.global.logJSON }} | ||
| -log-json={{ $root.Values.global.logJSON }} \ | ||
| {{- if (and $root.Values.global.metrics.enabled $root.Values.global.metrics.enableGatewayMetrics) }} | ||
| -telemetry-prom-scrape-path={{ $root.Values.connectInject.metrics.defaultPrometheusScrapePath }} |
There was a problem hiding this comment.
I think we need to hard-code this one to /metrics (that's how it worked before). This value is controlled by the value from the connectInject stanza which usually does not control gateways. Also, if someone changes this value, the prometheus annotations on the gateway deployment will not change because we're not consuming this value in the gateway annotations.
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
…mage names from testing
t-eckert
pushed a commit
that referenced
this pull request
Oct 31, 2022
Changes proposed in this PR: - Add support for merged metrics with consul-dataplane. - Merging metrics now takes place in the consul-dataplane itself instead of relying on the consul-sidecar - Pairs with a new flag added to consul-dataplane in PR: Add telemetry-prom-merge-port flag for merged metrics
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this PR:
This PR is best reviewed by commit(ish).
f9fe189 - passes metrics flags to the consul-dataplane binary.
ea62a71 - Re-enables the metrics acceptance tests and gets them running with consul-dataplane.
87630a7 - Removes consul-sidecar as it is no longer needed at all for merging metrics.
dde818d6536020 - Please ignore. Git/me being silly.
32cb989 - Update CHANGELOG.md
How I've tested this PR:
How I expect reviewers to test this PR:
👀
Checklist: