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

Release v1.3.0 #166

Closed
wants to merge 1 commit into from
Closed

Conversation

florianl
Copy link
Contributor

Release of the v1.3.0 version of the OTLP.

Release of the [v1.3.0][otlp] version of the OTLP.

[otlp]: https://github.com/open-telemetry/opentelemetry-proto/releases/tag/v1.3.0

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@florianl florianl requested a review from a team April 25, 2024 06:01
@pellared
Copy link
Member

This does not look completed. Changing to draft.

@pellared pellared marked this pull request as draft April 25, 2024 06:03
@florianl
Copy link
Contributor Author

hi @pellared
Thanks for the quick feedback. May I ask what you are missing?
I did follow opentelemetry-proto-go/RELEASING.md#create-a-release-pull-request.

@pellared
Copy link
Member

pellared commented Apr 25, 2024

I do not see the generated Go code. You need to update Makefile to reflect the changes done in github.com/open-telemetry/opentelemetry-proto so that the new code is generated.

It does not look great to add the experimental packages with v1.3.0. Should have separate versioning that contains Go code for the experimental proto? Personally, I do not think it would be a good idea as then the users would land into the https://protobuf.dev/reference/go/faq/#namespace-conflict issue like e.g. here: open-telemetry/opentelemetry-go#2579 (comment)

@florianl
Copy link
Contributor Author

Thanks - I see your point.
As PROTOBUF_VERSION := v1 in the Makefile does not cover v1experimental for the new profiles signal, no new code was generated.

What is your suggestion to resolve this situation and going forward?

@pellared
Copy link
Member

pellared commented Apr 25, 2024

It does not look great to add the experimental packages with v1.3.0.

We will probabably break SemVer if we decide to publish it. Should we publish the new signal as a separate Go module? I am not sure what is the best way to follow-up.

@florianl What is the reason that you created this PR? Are you planning to use the new profiles signal? If so then can you describe how?

@pellared
Copy link
Member

I added this topic to today's OTel Go SIG meeting agenda as I do not think it is clear how we want to follow up. You can join the meeting if you are able.

@florianl
Copy link
Contributor Author

florianl commented Apr 25, 2024

Are you planning to use the new profiles signal? If so then can you describe how?

Yes - the signal will be used in otel-profiling-agent This repository is proposed to be donated to Open Telemetry and the OTel Profiling SIG is working on integrating this signal into OTel Collector.
The generated Go code of the proto files is one step in this direction.

Currently, I'm updating the Makefile but run into the following error:

--grpc-gateway_out: HTTP rules without a matching selector: .opentelemetry.proto.collector.profiles.v1.ProfilesService.Export

Not sure yet, where v1 is coming from, where it should be v1experimental.

@pellared
Copy link
Member

pellared commented Apr 25, 2024

the signal will be used in otel-profiling-agent

If this is to be only used as a binary (and not as a library) then it may be more convenient to generate the code in https://github.com/elastic/otel-profiling-agent. It will give you more flexibility and reduce coupling. This is what https://github.com/open-telemetry/opentelemetry-collector does.

@dmathieu
Copy link
Member

It seems to like as long as opentelemetry-proto follows semver for future profiling changes, it shouldn't be so much of an issue for this repository.
When the proto repository does a patch release, we do that too. The same goes for patches.

@pellared
Copy link
Member

It seems to like as long as opentelemetry-proto follows semver for future profiling changes, it shouldn't be so much of an issue for this repository.

How are we sure that it will be followed? What if it gets stable? Will the experimental protos be kept? Removal would be a breaking change.

@dmathieu
Copy link
Member

Hmm right, based on the experimental signal status, they could possibly introduce breaking changes in the proto.

That goes in favor of not building the profiles into this repository until they are stable.

@christos68k
Copy link
Member

christos68k commented Apr 25, 2024

We will probabably break SemVer if we decide to publish it. Should we publish the new signal as a separate Go module? I am not sure what is the best way to follow-up.

Is there precedent for this, did you have to deal with other experimental signals in the past?

the signal will be used in otel-profiling-agent

If this is to be only used as a binary (and not as a library) then it may be more convenient to generate the code in https://github.com/elastic/otel-profiling-agent.

This is what we're currently doing but it involves either a source-code dependency (ie. git submodule) or manually copying the proto files. Besides us (Elastic), other implementors can now use the profiling signal and we expect to see the rate of adoption go up. It'd be very convenient to have the Go interface be a published library.

Is the separate Go module option realistic on your end?

@florianl florianl mentioned this pull request May 8, 2024
@florianl
Copy link
Contributor Author

florianl commented May 8, 2024

closing in favor of #170

@florianl florianl closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants