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 Prometheus scrape annotations to agent #684

Merged
merged 1 commit into from
Oct 8, 2019
Merged

Add Prometheus scrape annotations to agent #684

merged 1 commit into from
Oct 8, 2019

Conversation

nivaldogmelo
Copy link
Contributor

@nivaldogmelo nivaldogmelo commented Oct 6, 2019

As discussed in #28, it was created a way to insert prometheus annotations at agents that are deployed as sidecars so it won't overwrite similar annotations provided by the service.

Closes #28

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM

@jpkrohling jpkrohling changed the title ( #28) Add Prometheus scrape annotations to agent Add Prometheus scrape annotations to agent Oct 7, 2019
Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

LGTM - just one minor formatting issue to resolve.

@@ -160,7 +160,6 @@ func TestInjectSidecarWithEnvVarsOverridePropagation(t *testing.T) {
assert.Contains(t, dep.Spec.Template.Spec.Containers[0].Env, traceContextEnvVar)
assert.Contains(t, dep.Spec.Template.Spec.Containers[0].Env, corev1.EnvVar{Name: envVarServiceName, Value: "testapp.default"})
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can you restore this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Add default prometheus annotations to be inserted at sidecar in
case the deployment don't have any

Test(sidecar):Insertion of prometheus annotations

Add a test for a case with a deployment without any annotation and
another one for a deployment with annotations different from those
that we insert as default

Signed-off-by: Nivaldo Melo <nivaldogmelo@gmail.com>
@nivaldogmelo
Copy link
Contributor Author

The tests are now failing in a recipe for target vendor, that's strange since i only added a newline in a file. Locally i can run Make test but only after a remove the line replace git.apache.org/thrift.git => github.com/apache/thrift v0.12.0. Since on first PR the checks were a success without changing that file, i didn't pushed any changes to it.

@jpkrohling
Copy link
Contributor

It failed due to this:

go: vbom.ml/util@v0.0.0-20180919145318-efcd4e0f9787: unrecognized import path "vbom.ml/util" (https fetch: Get https://vbom.ml/util?go-get=1: dial tcp: lookup vbom.ml: Temporary failure in name resolution)

I'm restarting the jobs, hopefully the host is up again.

@jpkrohling
Copy link
Contributor

CI passed. Thanks for your contribution!

@jpkrohling jpkrohling merged commit 1e764a7 into jaegertracing:master Oct 8, 2019
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.

Add prometheus scrape annotations to agent
3 participants