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

Support exporting tracing data to oc_agent, then to datadog agent #3398

Merged
merged 3 commits into from
May 16, 2019

Conversation

gitlw
Copy link

@gitlw gitlw commented May 11, 2019

This change is Reviewable

@gitlw gitlw requested review from manishrjain and a team as code owners May 11, 2019 00:32
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @gitlw and @manishrjain)


ocagent/docker-compose.yml, line 73 at r1 (raw file):

volumes: {}
        

nit: new line


ocagent/ocagent-config.yaml, line 9 at r1 (raw file):

    namespace: "oc_pool"
    trace_addr: "datadog:8126"
    enable_tracing: true

nit: new line

@gitlw gitlw force-pushed the gitlw/add_oc_agent branch from 1db4eb0 to 390631e Compare May 11, 2019 00:35
@danielmai
Copy link
Contributor


ocagent/docker-compose.yml, line 17 at r2 (raw file):

--jaeger.collector=http://ocagent:14268

Does this work? I thought supporting the OpenCensus Agent requires a code change to export to the agent instead of Jaeger.

Copy link
Author

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)


ocagent/docker-compose.yml, line 17 at r2 (raw file):

Previously, danielmai (Daniel Mai) wrote…
--jaeger.collector=http://ocagent:14268

Does this work? I thought supporting the OpenCensus Agent requires a code change to export to the agent instead of Jaeger.

Yes, because the ocagent supports jaeger format receivers, as shown in the yaml config file in this PR.

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gitlw and @manishrjain)


ocagent/docker-compose.yml, line 17 at r2 (raw file):

Previously, gitlw (Lucas Wang) wrote…

Yes, because the ocagent supports jaeger format receivers, as shown in the yaml config file in this PR.

Interesting that this works. I think it's still worth migrating to the ocagent exporter to export both traces and metrics to different backends. That can be done in another PR.


ocagent/docker-compose.yml, line 66 at r3 (raw file):

      read_only: true
    environment:
    - DD_API_KEY=<key goes here>

This can just be - DD_API_KEY so that the environment variable can be picked up from the shell running docker-compose without modifying this docker-compose.yml.

https://docs.docker.com/compose/environment-variables/#pass-environment-variables-to-containers

Copy link
Contributor

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

:lgtm: One more comment from me. The rest LGTM.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gitlw and @manishrjain)

@gitlw gitlw merged commit 2a56d31 into master May 16, 2019
@gitlw gitlw deleted the gitlw/add_oc_agent branch May 16, 2019 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants