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.

Sign up for GitHub

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

Jump to bottom

Upgrade pdata to proto to 1.4.0 #11722

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Upgrade pdata to proto to 1.4.0 #11722

wants to merge 12 commits into from

Conversation

dmathieu
Copy link
Member

@dmathieu dmathieu commented Nov 21, 2024 •
edited
Loading

This upgrades pdata to the 1.4.0 version of protobufs.

Sorry for the very large PR. This can't really be split into smaller PRs though, as changes are not backwards compatible.

This PR includes #11706, as the two are very close and separating them would cause weird conflicts.
The two changes can be merged either as separate PRs (with 11706 first), or as one.

Closes #11720

cc @mx-psi

@codecov Codecov
Copy link

codecov bot commented Nov 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.99478% with 23 lines in your changes missing coverage. Please review.

Project coverage is 91.55%. Comparing base (c9f8e8b) to head (3306d59).

Files with missing lines Patch % Lines
pdata/pprofile/json.go 85.18% 8 Missing ⚠️
pdata/internal/generated_wrapper_int32slice.go 50.00% 6 Missing ⚠️
pdata/internal/generated_wrapper_intslice.go 50.00% 6 Missing ⚠️
pdata/pprofile/profile.go 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #11722      +/-   ##
==========================================
- Coverage   91.63%   91.55%   -0.09%     
==========================================
  Files         442      446       +4     
  Lines       23776    23726      -50     
==========================================
- Hits        21787    21722      -65     
- Misses       1618     1633      +15     
  Partials      371      371

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

@dmathieu dmathieu marked this pull request as ready for review November 21, 2024 14:51
@dmathieu dmathieu requested a review from a team as a code owner November 21, 2024 14:51
@mx-psi
Copy link
Member

mx-psi commented Nov 21, 2024

Is there any way we can make a graceful transition for avoiding this breakage in contrib?

=== FAIL: .  (0.00s)
FAIL	github.com/open-telemetry/opentelemetry-collector-contrib/extension/encoding/otlpencodingextension [build failed]

=== Errors
Error: ./extension_test.go:205:6: im.SetStartTime undefined (type pprofile.Profile has no field or method SetStartTime)
Error: ./extension_test.go:206:6: im.SetEndTime undefined (type pprofile.Profile has no field or method SetEndTime)

@dmathieu dmathieu mentioned this pull request Nov 21, 2024
Draft
@dmathieu
Copy link
Member Author

I can bring back StartTime, with TimeNanos as the origin field name. But I can't bring back EndTime, as Duration doesn't have the same meaning.

@dmathieu
Copy link
Member Author

I have made EndTime a noop.

@mx-psi
Copy link
Member

mx-psi commented Nov 21, 2024

This PR includes #11706, as the two are very close and separating them would cause weird conflicts.

Should we close #11706 then?

@dmathieu
Copy link
Member Author

It really depends how reviews go. If you prefer reviewing a single but bigger PR, then yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerHelmuth TylerHelmuth Awaiting requested review from TylerHelmuth TylerHelmuth is a code owner automatically assigned from open-telemetry/collector-approvers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade pdata to proto 1.4.0
2 participants
@dmathieu @mx-psi