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

[cmd/mdatagen] has been moved to opentelemetry-collector #30495

Closed

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Jan 12, 2024

Fixes #30497

Signed-off-by: Alex Boten <aboten@lightstep.com>
@github-actions github-actions bot added the cmd/mdatagen mdatagen command label Jan 12, 2024
@github-actions github-actions bot requested a review from dmitryax January 12, 2024 22:49
@dmitryax
Copy link
Member

@atoulme do we have any contrib's exclusive functionality in mdatagen right now. If so, we need to think of wrapping core's version instead of removing the whole module. I do agree we should not keep a copy of it here

@dmitryax
Copy link
Member

We can probably keep mdatagen experimental features in contrib for now. But eventually, they should all be moved to core and we need to remove this module

@atoulme
Copy link
Contributor

atoulme commented Jan 12, 2024

do we have any contrib's exclusive functionality in mdatagen right now

We do, quite a bit. All the test generation, I take it. As a matter of fact, the code changed today :)

I certainly hope we will follow a mindful deprecation cycle of this code rather than deleting it outright, right?

This deletion here doesn't address the go:generate instructions to generate code in modules, do we mean to add mdatagen through install_tools?

@codeboten
Copy link
Contributor Author

I certainly hope we will follow a mindful deprecation cycle of this code rather than deleting it outright, right?

I would expect this code to be moved to the core repo no?

This deletion here doesn't address the go:generate instructions to generate code in modules, do we mean to add mdatagen through install_tools?

Yes. That's the plan, this is still in draft.

@atoulme
Copy link
Contributor

atoulme commented Jan 12, 2024

I certainly hope we will follow a mindful deprecation cycle of this code rather than deleting it outright, right?

I would expect this code to be moved to the core repo no?

Yes, of course, since we're moving. But I'm pointing to the fact that per Go guidelines, we should first deprecate the package since it is published, see https://pkg.go.dev/github.com/AviatrixDev/opentelemetry-collector-contrib/cmd/mdatagen

And given our rate of change with mdatagen, I'd certainly appreciate if we can sync up and make sure we don't miss anything in the move.

This deletion here doesn't address the go:generate instructions to generate code in modules, do we mean to add mdatagen through install_tools?

Yes. That's the plan, this is still in draft.

Nice!

@codeboten
Copy link
Contributor Author

I'd certainly appreciate if we can sync up and make sure we don't miss anything in the move.

100% i opened this PR mostly not to forget to finish moving mdatagen :)

@atoulme
Copy link
Contributor

atoulme commented Jan 12, 2024

Filing #30497 to follow up and keeping track

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jan 27, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Feb 25, 2024
dmitryax added a commit that referenced this pull request Mar 6, 2024
…#31609)

This change migrates `generate` make target from using the deprecated
`cmd/mdatagen` in this repository to mdategen defined in core
repository.

To avoid breaking changes for the end users, we keep the scope names
used in this repo as before. This required defining them explicitly in
metadata.yaml files. We can update them after
open-telemetry/opentelemetry-collector#9494
and
#21469
are resolved.

Taking the opportunity that the scope names can be explicitly defined,
this PR also updates missing scope names for extensions with
inconsistent package names e.g.: `awsproxy` and `jaegerremotesampling`.
It's not a breaking change because the generated meter and tracer are
not being used yet.

This change unblocks
#30495
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 11, 2024
DougManton pushed a commit to DougManton/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
…open-telemetry#31609)

This change migrates `generate` make target from using the deprecated
`cmd/mdatagen` in this repository to mdategen defined in core
repository.

To avoid breaking changes for the end users, we keep the scope names
used in this repo as before. This required defining them explicitly in
metadata.yaml files. We can update them after
open-telemetry/opentelemetry-collector#9494
and
open-telemetry#21469
are resolved.

Taking the opportunity that the scope names can be explicitly defined,
this PR also updates missing scope names for extensions with
inconsistent package names e.g.: `awsproxy` and `jaegerremotesampling`.
It's not a breaking change because the generated meter and tracer are
not being used yet.

This change unblocks
open-telemetry#30495
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
…open-telemetry#31609)

This change migrates `generate` make target from using the deprecated
`cmd/mdatagen` in this repository to mdategen defined in core
repository.

To avoid breaking changes for the end users, we keep the scope names
used in this repo as before. This required defining them explicitly in
metadata.yaml files. We can update them after
open-telemetry/opentelemetry-collector#9494
and
open-telemetry#21469
are resolved.

Taking the opportunity that the scope names can be explicitly defined,
this PR also updates missing scope names for extensions with
inconsistent package names e.g.: `awsproxy` and `jaegerremotesampling`.
It's not a breaking change because the generated meter and tracer are
not being used yet.

This change unblocks
open-telemetry#30495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/mdatagen mdatagen command Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete cmd/mdatagen from contrib
3 participants