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

OTLP Examples fixed to be consistent #1752

Merged
merged 7 commits into from
May 14, 2024

Conversation

cijothomas
Copy link
Member

Fix the example to use same Resource for all signals.
The http one is now matching the grpc one, though I plan to send a follow up later to maintain single example with feature flag to control transport, as opposed to copying a lot.

Also, the http one is not working for logs, metrics. (this was same before this PR also, so not a blocker for this PR.)

@cijothomas cijothomas requested a review from a team as a code owner May 13, 2024 19:25
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.6%. Comparing base (348ec9e) to head (eaec230).
Report is 27 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1752     +/-   ##
=======================================
+ Coverage   71.0%   71.6%   +0.5%     
=======================================
  Files        135     136      +1     
  Lines      20751   20845     +94     
=======================================
+ Hits       14746   14931    +185     
+ Misses      6005    5914     -91     

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

@cijothomas
Copy link
Member Author

Also, the http one is not working for logs, metrics. (this was same before this PR also, so not a blocker for this PR.)

#1706 might have broken the endpoint.

@cijothomas cijothomas mentioned this pull request May 13, 2024
3 tasks
@TommyCpp
Copy link
Contributor

Also, the http one is not working for logs, metrics. (this was same before this PR also, so not a blocker for this PR.)

#1706 might have broken the endpoint.

The metrics endpoint should work as after #1706 we won't change anything from user input in builders. For logs, we need to manually append the /v1/logs path to the base URL

@cijothomas
Copy link
Member Author

Also, the http one is not working for logs, metrics. (this was same before this PR also, so not a blocker for this PR.)

#1706 might have broken the endpoint.

The metrics endpoint should work as after #1706 we won't change anything from user input in builders. For logs, we need to manually append the /v1/logs path to the base URL

Its still not working. :( Could you take a look ?

"Error: Other("[Other(reqwest::Error { kind: Status(400), url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(4318), path: "/v1/logs", query: None, fragment: None } })]")"

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM. Assuming the issue of OTLP HTTP Log Exporter would be handled separately.

@TommyCpp
Copy link
Contributor

Also, the http one is not working for logs, metrics. (this was same before this PR also, so not a blocker for this PR.)

#1706 might have broken the endpoint.

The metrics endpoint should work as after #1706 we won't change anything from user input in builders. For logs, we need to manually append the /v1/logs path to the base URL

Its still not working. :( Could you take a look ?

"Error: Other("[Other(reqwest::Error { kind: Status(400), url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(4318), path: "/v1/logs", query: None, fragment: None } })]")"

Yeah taking a look now

@lalitb
Copy link
Member

lalitb commented May 14, 2024

"Error: Other("[Other(reqwest::Error { kind: Status(400), url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(4318), path: "/v1/logs", query: None, fragment: None } })]")"

Bad Request, Could also be related to PR for sending Resource separately, and then combining before export. Let me check too :(

@TommyCpp
Copy link
Contributor

Alright I think there are some issues with OTLP http/json exporter. After set the http binary protocol everything works fine
cijothomas#1

@cijothomas cijothomas merged commit d2668e6 into open-telemetry:main May 14, 2024
20 checks passed
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.

None yet

3 participants