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

ddtrace/opentracer: consider FollowsFrom references as children #905

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

gbbr
Copy link
Contributor

@gbbr gbbr commented Apr 19, 2021

This change ensures that Opentracing spans using FollowsFrom references
will be considered as child spans.

Closes #376

This change ensures that Opentracing spans using FollowsFrom references
will be considered as child spans.

Closes #376
@gbbr gbbr added this to the 1.30.0 milestone Apr 19, 2021
@gbbr gbbr merged commit 2fbd304 into v1 Apr 19, 2021
@gbbr gbbr deleted the gbbr/follows-from branch April 19, 2021 08:54
stlimtat pushed a commit to stlimtat/dd-trace-go that referenced this pull request May 17, 2021
* 'v1' of https://github.com/DataDog/dd-trace-go:
  Implement DD_PROFILING_OUTPUT_DIR for dev/debug (DataDog#924)
  contrib/go-pg/pg.v10: add INTEGRATION flag check for tests (DataDog#921)
  contrib/go-redis/redis.v8: optimize BeforeProcess and BeforeProcessPipeline (DataDog#920)
  CI: check go.sum is up-to-date (DataDog#918)
  README: Fix go get instructions (DataDog#913)
  internal/log: Improve API for testing (DataDog#916)
  ddtrace/tracer: add support for agent discovery endpoint, feature flags, stats & drops (DataDog#859)
  internal/version: bump to v1.31.0 (DataDog#910)
  CONTRIBUTING.md: add link to contrib/README.md (DataDog#802)
  profiler: Disable agentless mode for WithAPIKey (DataDog#906)
  contrib/Shopify/sarama: fix possible deadlock in WrapAsyncProducer (DataDog#907)
  contrib/database/sql.tracedConn implement driver.SessionResetter (DataDog#908)
  ddtrace/opentracer: consider FollowsFrom references as children (DataDog#905)
  ddtrace/opentracer: add support for opentracing.TracerContextWithSpanExtension (DataDog#855)
  ddtrace/tracer: follow noDebugStack setting when using SetTag with an error (DataDog#900)
  contrib/net/http: add a getter to retrieve original round tripper (DataDog#903)
  ddtrace/tracer: ensure Flush call is synchronous (DataDog#901)
@itaysabato
Copy link

@gbbr May I ask why?

Excuse my bluntness, but I will give you an example of why I think this is a bad idea:

  • A classic FollowsFrom relationship would be an async task, e.g. publishing a message in a queue and having some other process do something with it.
  • While it is useful to be able to track this connection, having it considered a ChildFrom relationship creates a flame graph that is awful to look at, with ridiculously large gaps.
  • Coincidentally, this is exactly what I am experiencing since my queue publisher spans are automatically connected to my subscriber spans with a ChildFrom relationship.
  • I am in the midst of getting rid of this behavior and was hoping to replace it with a FollowsFrom relationship that better describes what is really happening - but given this change, I will have to disconnect the relationship completely.
  • Ideally, what I would want to see is full traces that start with a root span and display only true children sub-trees with the ability to "hop" to (or expand) "following" trace trees, and back. For searching and filtering, it would be useful to have this connection as well, e.g. find all parents/children of a trace/span.
  • Can this at least be made configurable/optional? This way I can create the correct relationship without worrying about how it will appear in the UI.

Thank you for listening!

@gbbr
Copy link
Contributor Author

gbbr commented Aug 18, 2021

Of course you may ask 🙂

We did this to be consistent. All the other tracers (in other languages) do the same thing. Datadog APM does not currently support this type of relationship so this is the default behaviour. If this is really an issue for you, we could maybe add a feature flag to disable it, but then FollowsFrom would simply become an invalid type of relation and spans wouldn't show up anymore as related (each span would be its own trace - no parent will be identified). I'm not sure if that outcome is more desirable than the current.

Let me know how you want to proceed.

P.S. You may want to try to create a feature request for adding full support of FollowsFrom relations in the UI using our support. The more people request it, the higher the chance that it becomes a reality. But I can't make any promises.

@itaysabato
Copy link

Thanks, I will try to open the feature request.

Given the current behavior, what I intend to do is make sure parent spans are disconnected from "non-child" spans but add the parent trace ID as a tag to the "non-child" and create a facet for it - this will allow me to manually hop between the traces.

If this could be the default behavior instead of treating them as real children it would be much easier for me. It would also be future proof, in case you do decide to use this relationship in another way. In any case, that's what I would do, if I were you :)

On another topic (somewhat related), do you have any ERs or plans to pursue the issue of searching tags from the root span (trace) together with an inner span?

I'll explain: the root of the trace usually contains the most useful contextual tags (e.g. the HTTP request tags and business context tags), but a lot of times I want to analyze a certain operation within that trace (e.g. how long a DB query takes on average) - this would be an inner span that does not have the contextual tags, unless I programmatically duplicate them, which is pretty painful.

So I would like to be able to query something like @trace.customer:acme.com @resource_name:get-orders @duration>=1000

Does that make any sense?

Thanks again.

@gbbr
Copy link
Contributor Author

gbbr commented Aug 19, 2021

Thanks for explaining. On the first part, with adding the trace ID as a tag on children. I don't think that's something we want to do. It feels like a hack to help a specific use-case work around limitations of the product. This isn't something we want to encourage moving forward or provide an option for at this point. On the other hand, advocating to add support for "follows-from" references is something worthwhile pursuing.

On your question on searching tags in the UI, that's a bit out of scope of this repository and I might not give the best answer. We are updating our product regularly with new features and interesting workflows, so I suggest also reaching out to our support for this case. They will be more than helpful and knowledgeable on the topic.

@gbbr
Copy link
Contributor Author

gbbr commented Aug 19, 2021

On the other hand, to not close all doors, to this suggestion:

Given the current behavior, what I intend to do is make sure parent spans are disconnected from "non-child" spans but add the parent trace ID as a tag to the "non-child" and create a facet for it - this will allow me to manually hop between the traces.

We may be able to add a feature flag which results in this behaviour. If I understand correctly, this feature flag would cause:

  • FollowsFrom references will no longer be children, they will be brand new traces (just like before this change)
  • We add the Span ID of the parent as a tag (maybe as follows_from: <span_id>)
  • You can then use the UI to connect these.

Is my assumption correct?

@itaysabato
Copy link

Yes, this is what I was going for - except I thought linking should be with trace_id because span_ids aren't really exposed or searchable in Datadog AFAIK. The problem is without linking to the exact span it would be difficult to know how the traces are linked.

I think what you described will work well if:

  1. Span Ids will be listed in the UI (where the tags are and in the tables)
  2. Span ids will be searchable.
  3. Both the parent span and parent trace ids will be tagged in the child.

It's actually a bit weird that you can search logs by trace ID (and not span ID) but you can't search traces by trace ID.

And on the more philosophical level - I don't see this as a hack because, as a data platform, I'd expect Datadog to have everything be exposed as data you can query/aggregate/observe first, and have the UI features come second. But that's just me :)

@itaysabato
Copy link

P.s. I would start using it right away even with just (3).

@gbbr
Copy link
Contributor Author

gbbr commented Aug 19, 2021

It's actually a bit weird that you can search logs by trace ID (and not span ID) but you can't search traces by trace ID.

I agree! Definitely mention this to support too. It might not be difficult to add, but this is just an assumption.

And on the more philosophical level - I don't see this as a hack because, as a data platform, I'd expect Datadog to have everything be exposed as data you can query/aggregate/observe first, and have the UI features come second. But that's just me :)

I agree with your point.

@gbbr
Copy link
Contributor Author

gbbr commented Aug 19, 2021

Maybe the tags could then be:

  • follows_from_span: <span_id>
  • follows_from_trace: <trace_id>

But frankly I don't like this too much because again it feels like a workaround. I'd rather wait to see how a support ticket asking for making Span IDs searchable works out first.

Does this sound like a reasonable plan to you?

@itaysabato
Copy link

I would go with:

  • follows_from: <span_id>
  • follows_from_trace: <trace_id>

And also

  • child_of: <span_id>
  • child_of_trace: <trace_id>

Since the relationships are defined for spans.

I don't see this as a workaround - I think tags are the (correct/only?) way to add searchable data to traces. Any additional features would be great, but I see this as a must. Of course span_id should also be there as a tag IMO - then it will be searchable without any additional features. IIRC the trace ID is just the span id of the root so there is no need for an additional tag.

@gbbr
Copy link
Contributor Author

gbbr commented Aug 25, 2021

I'm a bit on the fence here. I'm not against it but I want to make sure there is no other better solution. Let's wait for @knusbaum to share a 3rd point of view 🙏🏻

@itaysabato
Copy link

Thanks, I am trying to think how to make an enhancement request to your support out of this...

@gbbr
Copy link
Contributor Author

gbbr commented Aug 26, 2021

I’d say the feature request is full support for FollowsFrom relationships. Workarounds won’t take us too far.

@knusbaum
Copy link
Contributor

@itaysabato, @gbbr
I started typing here and ended up with too much. I opened an issue instead. Please see #993

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.

ddtrace/opentracer: unable to create FollowsFrom reference
3 participants