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 support for Datadog Tracing in the Executor and the Python Wrapper #2436

Closed
mwm5945 opened this issue Sep 14, 2020 · 11 comments
Closed

Add support for Datadog Tracing in the Executor and the Python Wrapper #2436

mwm5945 opened this issue Sep 14, 2020 · 11 comments
Assignees
Milestone

Comments

@mwm5945
Copy link
Contributor

mwm5945 commented Sep 14, 2020

Seldon could easily have support for Datadog in the Executor and the Python wrapper, as they already use OpenTracing. All that needs to be added is creating the DD tracer, and setting OpenTracer to use it.

@mwm5945 mwm5945 added the triage Needs to be triaged and prioritised accordingly label Sep 14, 2020
This was referenced Sep 14, 2020
@ukclivecox ukclivecox removed the triage Needs to be triaged and prioritised accordingly label Sep 17, 2020
@mwm5945
Copy link
Contributor Author

mwm5945 commented Oct 2, 2020

@cliveseldon do you think this will be scheduled for the next release?

@ukclivecox
Copy link
Contributor

Let me add it to 1.4 release project. We need final sign-off internally that is is not increasing our dependency base too much but it looks like as its following open tracing that it should make sense.

@ukclivecox ukclivecox added this to the 1.4 milestone Oct 2, 2020
@axsaucedo axsaucedo removed this from the 1.4 milestone Oct 15, 2020
@axsaucedo axsaucedo added this to the 1.6 milestone Oct 15, 2020
@mwm5945
Copy link
Contributor Author

mwm5945 commented Oct 22, 2020

@axsaucedo Just curious--why has this been moved so far out? The PR has been created already....
Thanks!

@axsaucedo
Copy link
Contributor

@mwm5945 we're currently debating the tradeoffs around adding these extra dependencies and maintenance of integration with Datadog, as currently our main tracing dependency is jaeger and prometheus for metrics. It seems that there is consensus that opentracing is something we'd want to explore adding but for datadog we'll have to explore further. Could you present this in the next community call? It would be quite useful to get further thoughts from the community around the datadog component as well.

@mwm5945
Copy link
Contributor Author

mwm5945 commented Oct 29, 2020

@axsaucedo thanks for the update! I'll try and make it (next?) thursday and present this. But, in case i can't, i'll just explain a bit here as well. It's my understanding that both the executor and the python wrapper already use open tracing (see here and here, both are using OpenTracing behind Jaeger). Just to make sure we're on the same page, OpenTracing allows you to instrument your code in a standard way, and use different tracers behind it. Forcing jaeger seems a bit restrictive, especially how prevalent Datadog is in the enterprise world.

@axsaucedo
Copy link
Contributor

@mwm5945 sounds great! Apologies for the delayed response, not srue I received a notification, but this sounds like a good next steps. It would be good to discuss this in the next community session, which is next week - would you be able to make it? You can find the agenda here, we'll add a point to discuss https://docs.seldon.io/projects/seldon-core/en/latest/developer/community.html

We have been discussing this internally and we agree that we'd want to move into opentracing, especially as jaeger seems to support the standard client. We can discuss further during the community call and flesh out some action points - we should be able to re-use / build upon the work you've done on the PR as well.

@mwm5945
Copy link
Contributor Author

mwm5945 commented Nov 12, 2020

@axsaucedo sure thing--i tried to join last week but got my time zones mixed up haha.

@axsaucedo
Copy link
Contributor

@mwm5945 no worries, ok sounds perfect - I'll add it to the agenda, looking forward. By the way are you on the community slack? We can also continue the discussion there.

@mwm5945
Copy link
Contributor Author

mwm5945 commented Nov 12, 2020

@axsaucedo I sure am!

@ukclivecox
Copy link
Contributor

Closing

@mwm5945
Copy link
Contributor Author

mwm5945 commented Jan 7, 2021

@cliveseldon just curious why this was closed? The PR hasn't been closed/merged either... Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants