datastreams: Port data-streams-go to dd-trace-go #2006

piochelepiotr · 2023-05-26T02:51:30Z

What does this PR do?

Move the repository https://github.com/DataDog/data-streams-go to the main APM Datadog Go repository.
This way, the two can be better integrated together and users only need to install 1 library.

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

Changed code has unit tests for its functionality.
If this interacts with the agent in a new way, a system test has been added.

pr-commenter · 2023-05-26T02:59:34Z

Benchmarks

Benchmark execution time: 2023-09-06 13:45:30

Comparing candidate commit 6341dc6 in PR branch piotr-wolski/add-data-streams-support with baseline commit b49b27c in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics.

ahmed-mez

As discussed offline, some core functionalities (like container ID parsing and config) need to be refactored first.

ahmed-mez · 2023-06-29T14:04:21Z

Hey @piochelepiotr ! Any update on this PR? It's been 2 months since the review, we'll probably consider it stale and close it if there is no progress in the next few weeks. Thanks!

datastreams/aggregator.go

datastreams/aggregator_test.go

ddtrace/ddtrace.go

ddtrace/internal/globaltracer_test.go

ddtrace/internal/noop_datastreams.go

ddtrace/internal/globaltracer.go

datastreams/transport.go

knusbaum

👋
I left an initial review, almost exclusively on how it integrates with the existing tracer.
I'll still have to look more closely at the data streams package itself.

I would like to understand better what this is, why dd-trace-go is a good place for it, and why it requires changes to the tracer itself, whereas previously it was able to exist as a stand-alone module.

ddtrace/ddtrace.go

ddtrace/tracer/data_streams.go

internal/statsd.go

ddtrace/tracer/tracer.go

ddtrace/tracer/option.go

datastreams/dsminterface/data_streams_interface.go

piochelepiotr · 2023-08-08T00:42:49Z

👋 I left an initial review, almost exclusively on how it integrates with the existing tracer. I'll still have to look more closely at the data streams package itself.

I would like to understand better what this is, why dd-trace-go is a good place for it, and why it requires changes to the tracer itself, whereas previously it was able to exist as a stand-alone module.

Thanks for the review!

So the reason to put everything here is that the next step is to use this library in all the integrations.
tracing Kafka for eg would automatically add data streams to Kafka.

The reason it requires to make changes to the tracer, is because I want tracer.Start() to also start the data streams processor.

ajgajg1134

I focused on the changes outside the new datastreams folder. Overall I think this seems like a good path forward to simplifying things for customers!

contrib/Shopify/sarama/data_streams.go

ddtrace/tracer/tracer.go

ddtrace/tracer/mockdatastreams.go

ajgajg1134

Definitely a bit nicer for customers to reduce the number of interfaces needed here! Just a few smaller change requests / questions :)

contrib/Shopify/sarama/sarama.go

contrib/Shopify/sarama/sarama_test.go

contrib/confluentinc/confluent-kafka-go/kafka/kafka.go

contrib/confluentinc/confluent-kafka-go/kafka/kafka_test.go

knusbaum

Some comments to consider, but overall this is a 👍 from me.

contrib/Shopify/sarama/option.go

knusbaum · 2023-08-31T17:23:33Z

ddtrace/tracer/data_streams.go

+// SetDataStreamsCheckpoint sets a consume or produce checkpoint in a Data Streams pathway.
+// This enables tracking data flow & end to end latency.
+// To learn more about the data streams product, see: https://docs.datadoghq.com/data_streams/go/
+func SetDataStreamsCheckpoint(ctx context.Context, edgeTags ...string) (p datastreams.Pathway, outCtx context.Context, ok bool) {
+	return SetDataStreamsCheckpointWithParams(ctx, datastreams.NewCheckpointParams(), edgeTags...)
+}
+
+// SetDataStreamsCheckpointWithParams sets a consume or produce checkpoint in a Data Streams pathway.
+// This enables tracking data flow & end to end latency.
+// To learn more about the data streams product, see: https://docs.datadoghq.com/data_streams/go/
+func SetDataStreamsCheckpointWithParams(ctx context.Context, params datastreams.CheckpointParams, edgeTags ...string) (p datastreams.Pathway, outCtx context.Context, ok bool) {
+	if t, ok := internal.GetGlobalTracer().(datastreams.ProcessorContainer); ok {
+		if processor := t.GetDataStreamsProcessor(); processor != nil {
+			p, outCtx = processor.SetCheckpointWithParams(ctx, params, edgeTags...)
+			return p, outCtx, true
+		}
+	}
+	return datastreams.Pathway{}, ctx, false
+}
+
+// TrackKafkaCommitOffset should be used in the consumer, to track when it acks offset.
+// if used together with TrackKafkaProduceOffset it can generate a Kafka lag in seconds metric.
+func TrackKafkaCommitOffset(group, topic string, partition int32, offset int64) {
+	if t, ok := internal.GetGlobalTracer().(datastreams.ProcessorContainer); ok {
+		if p := t.GetDataStreamsProcessor(); p != nil {
+			p.TrackKafkaCommitOffset(group, topic, partition, offset)
+		}
+	}
+}
+
+// TrackKafkaProduceOffset should be used in the producer, to track when it produces a message.
+// if used together with TrackKafkaCommitOffset it can generate a Kafka lag in seconds metric.
+func TrackKafkaProduceOffset(topic string, partition int32, offset int64) {


This looks fine to me, but to be clear, this is becoming a public API, even though we are really only using it within the module right now, and we don't really expect users to directly use it.

The consequence of this is that this API cannot change at all once we release it.
If this is not a stable, complete API, and is still being developed at all, it may be better to find some way to hide it so it's not exposed publicly.

Yeah. given that not everyone uses the sarama or confluent clients, and not everyone stores offsets in Kafka, I think it's important to leave that options to customers.
The main change I see for this API, is an additional argument (Kafka cluster) that we might add in the future.
Maybe I can add it directly, but ignore it for now?

actually, I don't know what would be the API today. maybe a list of strings, maybe just one string.
So I think what we can do is add another function, but leave this one once we know how we will track clusters.

CODEOWNERS

contrib/confluentinc/confluent-kafka-go/kafka.v2/option.go

ahmed-mez · 2023-09-01T08:32:18Z

datastreams/context.go

+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2016-present Datadog, Inc.
+
+package datastreams


Have you considered putting the public types and functions in a datastreams/internal package instead of exposing everything to users in the current datastreams package ? By having an internal package you'll be able to change it freely in the future without any backward-compatibility restrictions. This also a common practice in dd-trace-go, see ddtrace/internal, profiler/internal and contrib/internal.

datastreams/processor.go

datastreams/pathway.go

contrib/confluentinc/confluent-kafka-go/kafka.v2/kafka.go

contrib/Shopify/sarama/sarama.go

ddtrace/tracer/data_streams.go

Pulled the changes from DataDog#2006 into the feature branch

piochelepiotr force-pushed the piotr-wolski/add-data-streams-support branch 3 times, most recently from c1fac6c to 0448e4d Compare May 26, 2023 14:22

piochelepiotr marked this pull request as ready for review May 26, 2023 14:40

piochelepiotr requested a review from a team May 26, 2023 14:40

piochelepiotr requested a review from a team as a code owner May 26, 2023 14:40

hokitam approved these changes May 26, 2023

View reviewed changes

ahmed-mez requested changes May 30, 2023

View reviewed changes

knusbaum assigned ahmed-mez Jun 29, 2023

piochelepiotr force-pushed the piotr-wolski/add-data-streams-support branch from 0448e4d to 3d8ee19 Compare August 2, 2023 19:35

github-actions bot reviewed Aug 2, 2023

View reviewed changes

datastreams/aggregator.go Outdated Show resolved Hide resolved

datastreams/aggregator.go Outdated Show resolved Hide resolved

datastreams/aggregator_test.go Outdated Show resolved Hide resolved

github-actions bot reviewed Aug 2, 2023

View reviewed changes

datastreams/aggregator_test.go Outdated Show resolved Hide resolved

github-actions bot reviewed Aug 3, 2023

View reviewed changes

ddtrace/ddtrace.go Outdated Show resolved Hide resolved

ddtrace/internal/globaltracer_test.go Outdated Show resolved Hide resolved

ddtrace/internal/noop_datastreams.go Outdated Show resolved Hide resolved

piochelepiotr force-pushed the piotr-wolski/add-data-streams-support branch 2 times, most recently from 7735514 to 386f073 Compare August 3, 2023 04:20

github-actions bot reviewed Aug 3, 2023

View reviewed changes

ddtrace/internal/globaltracer.go Outdated Show resolved Hide resolved

datastreams/transport.go Outdated Show resolved Hide resolved

piochelepiotr force-pushed the piotr-wolski/add-data-streams-support branch 2 times, most recently from d9f9cfc to ca184db Compare August 3, 2023 14:18

piochelepiotr requested review from ahmed-mez and hokitam August 3, 2023 16:53

knusbaum requested changes Aug 7, 2023

View reviewed changes

ahmed-mez removed their assignment Aug 8, 2023

ahmed-mez removed their request for review August 8, 2023 08:08

ajgajg1134 requested changes Aug 23, 2023

View reviewed changes

piochelepiotr force-pushed the piotr-wolski/add-data-streams-support branch 2 times, most recently from 3e2fccc to bb750a9 Compare August 25, 2023 20:41

ajgajg1134 requested changes Aug 28, 2023

View reviewed changes

merge with main

61202af

ajgajg1134 approved these changes Aug 31, 2023

View reviewed changes

Merge branch 'main' into piotr-wolski/add-data-streams-support

4c9d971

knusbaum approved these changes Aug 31, 2023

View reviewed changes

piochelepiotr added 2 commits August 31, 2023 13:52

fix data streams processor test

9d1c5ad

update comment

24419a9

piochelepiotr requested a review from ahmed-mez August 31, 2023 19:03

ahmed-mez approved these changes Sep 1, 2023

View reviewed changes

felixge reviewed Sep 4, 2023

View reviewed changes

datastreams/processor.go Outdated Show resolved Hide resolved

piochelepiotr added 3 commits September 5, 2023 11:52

[data streams] Move to internal package

c6563aa

update comments

9692a0f

merge with main

1e5ec5e

github-actions bot reviewed Sep 5, 2023

View reviewed changes

datastreams/pathway.go Show resolved Hide resolved

contrib/confluentinc/confluent-kafka-go/kafka.v2/kafka.go Outdated Show resolved Hide resolved

contrib/confluentinc/confluent-kafka-go/kafka.v2/kafka.go Show resolved Hide resolved

piochelepiotr added 2 commits September 5, 2023 12:02

copy kafka.v2

2f71a6d

import

b3ecfe1

github-actions bot reviewed Sep 5, 2023

View reviewed changes

contrib/Shopify/sarama/sarama.go Outdated Show resolved Hide resolved

contrib/Shopify/sarama/sarama.go Show resolved Hide resolved

update

388754e

github-actions bot reviewed Sep 5, 2023

View reviewed changes

ddtrace/tracer/data_streams.go Show resolved Hide resolved

piochelepiotr added 2 commits September 5, 2023 13:50

fix copyright

c94ebe9

switch to base64 encoding

6b36a9c

piochelepiotr force-pushed the piotr-wolski/add-data-streams-support branch from 07a9894 to 6b36a9c Compare September 5, 2023 19:35

piochelepiotr added 4 commits September 5, 2023 17:49

use default statsd client

cd82b27

fix committed offsets tracking

a665739

add internal folder to data streams

80990f1

merge with main

6341dc6

ajgajg1134 merged commit 3ebb83f into main Sep 6, 2023
52 checks passed

ajgajg1134 deleted the piotr-wolski/add-data-streams-support branch September 6, 2023 13:54

KyleWiering mentioned this pull request Feb 26, 2024

v2/contrib: Add datastreams support for contrib/IBM/sarama #2573

Open

KyleWiering added a commit to KyleWiering/dd-trace-go that referenced this pull request Feb 26, 2024

Fixes DataDog#2573

127cb00

Pulled the changes from DataDog#2006 into the feature branch

KyleWiering mentioned this pull request Feb 26, 2024

v2/contrib/IBM/sarama Feature parity with deprecated shopify/sarama datastreams #2574

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datastreams: Port data-streams-go to dd-trace-go #2006

datastreams: Port data-streams-go to dd-trace-go #2006

piochelepiotr commented May 26, 2023 •

edited

Loading

pr-commenter bot commented May 26, 2023 •

edited

Loading

ahmed-mez left a comment

ahmed-mez commented Jun 29, 2023

knusbaum left a comment

piochelepiotr commented Aug 8, 2023

ajgajg1134 left a comment

ajgajg1134 left a comment

knusbaum left a comment

knusbaum Aug 31, 2023

piochelepiotr Aug 31, 2023

piochelepiotr Aug 31, 2023

ahmed-mez Sep 1, 2023

datastreams: Port data-streams-go to dd-trace-go #2006

datastreams: Port data-streams-go to dd-trace-go #2006

Conversation

piochelepiotr commented May 26, 2023 • edited Loading

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

pr-commenter bot commented May 26, 2023 • edited Loading

Benchmarks

ahmed-mez left a comment

Choose a reason for hiding this comment

ahmed-mez commented Jun 29, 2023

knusbaum left a comment

Choose a reason for hiding this comment

piochelepiotr commented Aug 8, 2023

ajgajg1134 left a comment

Choose a reason for hiding this comment

ajgajg1134 left a comment

Choose a reason for hiding this comment

knusbaum left a comment

Choose a reason for hiding this comment

knusbaum Aug 31, 2023

Choose a reason for hiding this comment

piochelepiotr Aug 31, 2023

Choose a reason for hiding this comment

piochelepiotr Aug 31, 2023

Choose a reason for hiding this comment

ahmed-mez Sep 1, 2023

Choose a reason for hiding this comment

piochelepiotr commented May 26, 2023 •

edited

Loading

pr-commenter bot commented May 26, 2023 •

edited

Loading