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

contrib/Shopify/sarama: Fix WrapAsyncProducer tracing support #738

Merged
merged 5 commits into from
Nov 4, 2020

Conversation

adw1n
Copy link
Contributor

@adw1n adw1n commented Sep 21, 2020

This is a continuation of now abandoned PR #487 by @HatsuneMiku3939

Closes #483

Quick recap:

  1. Currently WrapAsyncProducer doesn't work at all. Like explained in contrib/Shopify/sarama: bug in AsyncProducer #483, here
    key := spanKey{msg.Topic, msg.Partition, msg.Offset}
    both msg.Partition and msg.Offset are always 0. Here
    key := spanKey{msg.Topic, msg.Partition, msg.Offset}
    msg.Partition and msg.Offset are set correctly and this line
    finishProducerSpan(span, msg.Partition, msg.Offset, nil)
    is never reached.
  2. Kafka 0.11.0 was released on Jun 28, 2017 https://github.com/apache/kafka/releases/tag/0.11.0.0 Many open source projects default to much newer versions, e.g. Goka defaults to 2.0.0 https://github.com/lovoo/goka/blob/3d39c5afcd0a0d967c49318208e2b313bc0dfc8c/config.go#L31 As long as we document that WrapAsyncProducer requires at least 0.11.0 no one should hold a grudge. Original WrapAsyncProducer author explains the need for Kafka 0.11.0 here contrib/Shopify/sarama: add support for tracing the sarama kafka package #296 (comment)
  3. Sadly fixing TestAsyncProducer tests is really problematic because of broken sarama.MockBroker for newer kafka versions. You'll see this error when you don't skip the tests:
    TestAsyncProducer/With_Successes: sarama_test.go:246: kafka: client has run out of available brokers to talk to (Is your cluster reachable?
    
    I think that practicality matters and we should fix the WrapAsyncProducer even if we can't cover it with tests. Code changes in this PR should be pretty straightforward.

HatsuneMiku3939 and others added 2 commits September 21, 2020 17:24
* only successfully transmitted message have valid Partition, Offset
@adw1n
Copy link
Contributor Author

adw1n commented Sep 27, 2020

@dd-caleb @gbbr please could you review this PR?

@knusbaum
Copy link
Contributor

@adw1n I can take a look at this today.

@knusbaum knusbaum added this to the 1.28.0 milestone Sep 28, 2020
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

Aside from my minor comment, this looks basically good to me.

Thanks for linking the upstream issues for the mock tracer. I'm not happy about not being able to run the unit tests, so hopefully that can be fixed upstream soon. I'm going to spend a bit of time looking at the mock broker issue to make sure I understand it, and run some manual tests.

Seeing as a working integration with skipped tests is better than a broken integration with tests, we can still move forward even if we can't run the tests yet, provided we open an issue to track the upstream fix so we re-enable the tests when we are able.

contrib/Shopify/sarama/sarama.go Outdated Show resolved Hide resolved
@adw1n adw1n requested a review from knusbaum September 29, 2020 11:16
@adw1n
Copy link
Contributor Author

adw1n commented Oct 26, 2020

@knusbaum please could you have another look at this PR.

Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

@adw1n Thank you for being patient with this PR.
I ran manual testing on this and it looks good.

We should follow-up on the mock broker in the future or consider adding an integration test.

I have just one minor note about the documentation string and it should be OK. If you don't have time, I can modify the doc.

As a note, there may be conflicts with #761

contrib/Shopify/sarama/sarama.go Show resolved Hide resolved
@adw1n
Copy link
Contributor Author

adw1n commented Nov 4, 2020

@knusbaum I resolved the conflict you are referring to a week ago.

@adw1n adw1n requested a review from knusbaum November 4, 2020 00:10
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

LGTM.
Again, thanks for being patient with this one @adw1n

@knusbaum knusbaum merged commit 1ef07f6 into DataDog:v1 Nov 4, 2020
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.

contrib/Shopify/sarama: bug in AsyncProducer
3 participants