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

feat: Add Opentelemetry Export protocol option #2318

Closed
wants to merge 11 commits into from
Closed

feat: Add Opentelemetry Export protocol option #2318

wants to merge 11 commits into from

Conversation

bunkrur
Copy link

@bunkrur bunkrur commented Jun 8, 2023

Relates to #2316

Changed: AddOpenTelemetryCollectorMetricSink now takes all Otel Options
Added:  options.Protocol to OpenTelemetry exporter. Defaults to Grpc if http not found in config. Grpc is already default.
@bunkrur bunkrur requested a review from tomkerkhove as a code owner June 8, 2023 18:51
@trafico-bot trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Jun 8, 2023
@bunkrur bunkrur changed the title Add Opentelemetry Export protocol option feat: Add Opentelemetry Export protocol option Jun 8, 2023
Copy link
Owner

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Added some comments. Would you mind also updating the docs please? https:/promitor/docs

Changed: To equals from Startswith. Lets be explicit.
Added: Validation for protocol, even if using default
Added: Forcing grpc (which is already default) within CollectorInfo properties.
Added: Nested CollectorInfo class
Copy link
Owner

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Thanks! Added a few comments to polish it


public class CollectorInfo
{
public string CollectorProtocol { get; set; } = "grpc";
Copy link
Owner

Choose a reason for hiding this comment

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

Let's introduce Uri here as well to be consistent and deprecate the old one

Copy link
Author

Choose a reason for hiding this comment

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

Added to new class but kept filename :)

Copy link
Author

Choose a reason for hiding this comment

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

see other comment about class after I nuked the unit tests. Also, a change to the class structure or a nested class would maybe need runtime.yaml changes, which I think we should avoid?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ bunkrur
❌ Sameer Nafdey


Sameer Nafdey seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@bunkrur bunkrur closed this Jun 12, 2023
@bunkrur
Copy link
Author

bunkrur commented Jun 12, 2023

Closing, will reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔍 Ready for Review Pull Request is not reviewed yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants