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

[exporter/datadog] Deprecate service and version settings #8784

Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Mar 22, 2022

Description:

Deprecate service and version settings in favor of using the service.name and service.version semantic conventions respectively.

Link to tracking Issue: #8781 and #8783

Testing: Tested that deprecation warnings are logged when using these settings.

Documentation: Removed service and version from examples.

@mx-psi mx-psi force-pushed the mx-psi/deprecate-service-version branch from 35ff8f8 to 0775893 Compare March 22, 2022 12:33
@mx-psi mx-psi marked this pull request as ready for review March 22, 2022 12:34
@mx-psi mx-psi requested review from a team and dashpole March 22, 2022 12:34
Copy link
Contributor

@KSerrania KSerrania left a comment

Choose a reason for hiding this comment

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

Left a question, otherwise LGTM.

Comment on lines 11 to 12
service: myservice
version: myversion
Copy link
Contributor

Choose a reason for hiding this comment

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

We're deprecating these options, but they're technically still supported until v0.51.0. Should we keep the tests until we definitely remove these options (to prevent breaking the behavior of these options before the scheduled date)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added back this with a comment stating they are deprecated. I didn't do this originally because you need to check Config fields individually until we do #8373 (since the warnings field is unexported)

@mx-psi mx-psi added the ready to merge Code review completed; ready to merge by maintainers label Mar 24, 2022
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts

@mx-psi
Copy link
Member Author

mx-psi commented Mar 25, 2022

Done!

@mx-psi
Copy link
Member Author

mx-psi commented Mar 29, 2022

@codeboten could this be merged if it looks good? I fixed the conflicts

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Sorry @mx-psi, I missed this message and it looks like there are more conflicts to resolve :(

@mx-psi
Copy link
Member Author

mx-psi commented Mar 29, 2022

Should be good now :)

@jpkrohling
Copy link
Member

There's a release in progress, so I shouldn't merge this. After the release, this PR will likely get outdated (changelog, go.mod/sum).

This won't make it into v0.48.0, so I updated the deprecation and removal versions
@mx-psi mx-psi removed the ready to merge Code review completed; ready to merge by maintainers label Mar 30, 2022
@mx-psi
Copy link
Member Author

mx-psi commented Mar 30, 2022

Alright, will remove the 'ready to merge' label then until after v0.48.0 is done

@mx-psi mx-psi added the ready to merge Code review completed; ready to merge by maintainers label Mar 31, 2022
@mx-psi
Copy link
Member Author

mx-psi commented Mar 31, 2022

v0.48.0 is done, so I merged main into this branch, updated the CHANGELOG and marked as ready to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants