contrib/IBM/sarama: adds trace for IBM/sarama #2142
Conversation
contrib/IBM/sarama/sarama.go
Outdated
| @@ -0,0 +1,304 @@ | |||
| // Unless explicitly stated otherwise all files in this repository are licensed | |||
There was a problem hiding this comment.
The path for this needs to be contrib/IBM/sarama.v1/
There was a problem hiding this comment.
I've updated the path.
go.mod
Outdated
| @@ -12,7 +12,8 @@ require ( | |||
| github.com/DataDog/go-libddwaf v1.4.1 | |||
| github.com/DataDog/gostackparse v0.5.0 | |||
| github.com/DataDog/sketches-go v1.2.1 | |||
| github.com/Shopify/sarama v1.22.0 | |||
| github.com/IBM/sarama v1.40.0 | |||
| github.com/Shopify/sarama v1.19.0 | |||
There was a problem hiding this comment.
Is there a reason to downgrade this dependency?
We should avoid doing this unless necessary.
It looks like this might be causing the CI to fail.
There was a problem hiding this comment.
No reason, that was unintentional. The original version was restored.
contrib/IBM/sarama/sarama.go
Outdated
| const componentName = "IBM/sarama" | ||
|
|
||
| func init() { | ||
| telemetry.LoadIntegration("IBM/sarama") |
There was a problem hiding this comment.
We want to add a tracer.MarkIntegrationImported("github.com/IBM/sarama") line here like we do in the current sarama package.
There also needs to be an entry in ddtrace/tracer/option.go in the contribIntegrations map.
rodrigoff
force-pushed
the
add-contrib-ibm-sarama
branch
2 times, most recently
from
9af3449
to
42f471c
Compare
rodrigoff
force-pushed
the
add-contrib-ibm-sarama
branch
from
42f471c
to
47a7980
Compare
|
@knusbaum Hey, I've taken care of the issues you pointed out and rebased to the latest version. Please review the changes again whenever you get the chance :) |
contrib/IBM/sarama.v1/sarama.go
Outdated
| const componentName = "IBM/sarama" | ||
|
|
||
| func init() { | ||
| telemetry.LoadIntegration("IBM/sarama") |
There was a problem hiding this comment.
To keep things consistent with the other contrib packages, let's use telemetry.LoadIntegration(componentName) here instead.
ddtrace/tracer/option.go
Outdated
| "github.com/IBM/sarama": {"Kafka (sarama)", false}, | ||
| "github.com/Shopify/sarama": {"Kafka (sarama)", false}, |
There was a problem hiding this comment.
Having two integrations called "Kafka (sarama)" might be misleading. Can we change them to better represent what contrib integrations they come from? I.e., "IBM sarama" and "Shopify sarama" or similar
There was a problem hiding this comment.
I agree, I was just trying to keep consistency with confluent-kafka-go which is named Kafka (confluent) (and Kafka (confluent) v2, but looking at other contrib names, I don't think that's necessary.
I've updated both names to better represent their integration.
As of version 1.40.0 and moving forward, sarama's ownership has been transfered from Shopify to IBM. This change adds the contrib package for the new package as a copy of contrib/Shopify/sarama (in order not to break anything) to allow for apps to migrate to the new package.
rodrigoff
force-pushed
the
add-contrib-ibm-sarama
branch
from
47a7980
to
727d941
Compare
What does this PR do?
As of version 1.40.0 and moving forward, sarama's ownership has been transfered from Shopify to IBM.
This change adds the contrib package for the new package as a copy of contrib/Shopify/sarama (in order not to break anything) to allow for apps to migrate to the new package.
Motivation
See IBM/sarama#2461 for more details
Describe how to test/QA your changes
Reviewer's Checklist