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

[otlp] update protobuf definitions to new location/version in datadog-agent repo #103

Merged
merged 9 commits into from
Jun 14, 2023

Conversation

truthbk
Copy link
Member

@truthbk truthbk commented Jun 14, 2023

What does this PR do?

This PR updates the protobuf imports such that the new definitions in pkg/proto in the agent repository are used. This includes some changes due to the gogoproto nullable type being removed from the definitions.

Additionally, due to a chicken-egg problem, these changes are currently pointing to a WIP PR on the datadog-agent repository (PR to be posted soon). Unfortunately the current protobuf structure (definitions part of https://github.com/DataDog/datadog-agent) doesn't make it easy to make these updates.

Motivation

Refactor of protobuf definitions in https://github.com/DataDog/datadog-agent so that all definitions now live in pkg/proto as opposed to scattered all over the repository.

@truthbk truthbk requested a review from a team as a code owner June 14, 2023 11:33
@truthbk truthbk requested a review from liustanley June 14, 2023 11:33
@truthbk truthbk changed the title Jaime/protorefactor [otlp] update protobuf definitions to new location/version in datadog-agent repo Jun 14, 2023
@truthbk truthbk force-pushed the jaime/protorefactor branch 2 times, most recently from e51ba86 to 00d8a5c Compare June 14, 2023 11:53
@songy23 songy23 added the breaking-change This PR introduces a Go API breaking change label Jun 14, 2023
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

I verified make gen-licenses passed locally but failed on CI, #104 to follow up

@@ -145,7 +145,7 @@ pkg/otlp/attributes,google.golang.org/protobuf/types/descriptorpb,BSD-3-Clause,C
pkg/otlp/attributes,google.golang.org/protobuf/types/known/anypb,BSD-3-Clause,Copyright (c) 2018 The Go Authors. All rights reserved
pkg/otlp/attributes,google.golang.org/protobuf/types/known/durationpb,BSD-3-Clause,Copyright (c) 2018 The Go Authors. All rights reserved
pkg/otlp/attributes,google.golang.org/protobuf/types/known/timestamppb,BSD-3-Clause,Copyright (c) 2018 The Go Authors. All rights reserved
pkg/otlp/metrics,github.com/DataDog/datadog-agent/pkg/trace/pb,Apache-2.0,"Copyright 2016-present Datadog, Inc"
pkg/otlp/metrics,github.com/DataDog/datadog-agent/pkg/proto/pbgo/trace,Apache-2.0,"Copyright 2016-present Datadog, Inc"
Copy link
Member

Choose a reason for hiding this comment

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

This is the only change needed in this file, please revert other changes.

.github/workflows/test.yaml Outdated Show resolved Hide resolved
@songy23 songy23 merged commit dab6226 into main Jun 14, 2023
@songy23 songy23 deleted the jaime/protorefactor branch June 14, 2023 17:00
@gbbr
Copy link
Contributor

gbbr commented Jun 16, 2023

Can we revert this change? This package doesn't even exist on main (https://github.com/DataDog/datadog-agent/tree/main/pkg/proto/pbgo), and we can't update the exporter anymore to use this code because it depends on pkg/trace from the datadog-agent, which isn't updated. So we are technically blocked.

We can keep the PR open for when it's ready but I don't think we should have merged it.

gbbr added a commit that referenced this pull request Jun 16, 2023
@gbbr gbbr mentioned this pull request Jun 16, 2023
gbbr added a commit that referenced this pull request Jun 16, 2023
@songy23
Copy link
Member

songy23 commented Jun 16, 2023

I believe this needs to be merged to break a circular dependency issue (?) in Agent because @truthbk needs a pseudo version of mapping go with this change and use it in Agent. But if this is blocking the upgrade in datadog exporter, I agree we should revert it for now.

@gbbr
Copy link
Contributor

gbbr commented Jun 16, 2023

Sure, but this can be referenced elsewhere as a commit from this (unmerged) PR so it should be fine for Jaime (I think?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This PR introduces a Go API breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants