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

[exporterhelper] Expose queue consumer id or allow providing consumer factory #2254

Closed
mx-psi opened this issue Dec 4, 2020 · 4 comments
Closed

Comments

@mx-psi
Copy link
Member

mx-psi commented Dec 4, 2020

Is your feature request related to a problem? Please describe.

The Datadog exporter needs to use a certain struct for exporting traces. This struct can be reused across calls to pushTraceData and we currently have a single struct which is reused (with the help of a mutex) across all pushTraceData calls from all consumers. To avoid the mutex we would benefit from using one of these structs per consumer of the queue defined in the exporterhelper, but this is currently not possible given the public interface of this package.

Describe the solution you'd like

I see two possible approaches for this. Both solutions would necessarily mean changing the bounded queue used by the exporterhelper, either with a change to the package being used or by replacing it with a different one. Right now all consumers need to be the same:

func (qrs *queuedRetrySender) start() {
qrs.queue.StartConsumers(qrs.cfg.NumConsumers, func(item interface{}) {
req := item.(request)
_, _ = qrs.consumerSender.send(req)
})
}

Solution 1

Provide a way for passing a factory of queue consumers to the exporterhelper. In our use case these consumers would wrap the exporter and have a copy of the struct, which they could pass to the exporter.

Solution 2

Expose an id for each consumer. Each consumer would have an identifier that would be exposed to a pushTraceData-like function. The exporter would then handle resources in an array or map and retrieve them by id.

@andrewhsu
Copy link
Member

from the triage session, assigning to @tigrannajaryan to look into

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Dec 9, 2020

It seems like you have requirements that are specialized enough to warrant your own implementation and avoid the exporterhelper.
I fail to see how the proposed changes are useful generally for all exporters. Perhaps you can show on a draft PR.

@mx-psi
Copy link
Member Author

mx-psi commented Dec 10, 2020

Thanks for having a look. I will try to discuss this with the Jaeger maintainers first to see if their bounded queue could be updated to fit this use case.

@mx-psi
Copy link
Member Author

mx-psi commented Feb 24, 2021

Hi all, jaegertracing/jaeger#2714 is available on Jaeger 1.22 which was released just today and would allow me to continue working on this enhacenment.

However, we (Datadog) no longer need to use this enhacenment to solve the problem that motivated it in the first place: as can be seen in DataDog/datadog-agent#7450 (merged just today) we are moving this logic to the backend in all our clients (and eventually in the Datadog exporter too), thus solving our problem.

I am going to therefore close this issue; if there is interest by anyone else in the community to reopen this, the features added by the above Jaeger PR can be useful as a starting point.

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

No branches or pull requests

3 participants