Skip to content

Abstract out OTLP http exporter logic#5160

Open
lorenzoronzani wants to merge 27 commits into
open-telemetry:mainfrom
lorenzoronzani:refactor/http-exporter-logic
Open

Abstract out OTLP http exporter logic#5160
lorenzoronzani wants to merge 27 commits into
open-telemetry:mainfrom
lorenzoronzani:refactor/http-exporter-logic

Conversation

@lorenzoronzani
Copy link
Copy Markdown

@lorenzoronzani lorenzoronzani commented Apr 29, 2026 •
edited
Loading

Description

This PR contains the refactoring required inside the http_exporter_logic component. The idea is to remove the duplicated code abstracting out inside a common section

Fixes #2990

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (functionalities are the same, interfaces also but the logic is moved inside codebase)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Executed component specific unit tests using the following command uv run tox -e py312-test-opentelemetry-exporter-otlp-proto-http

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No. No, it's just an internal refactoring

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added/updated
  • Documentation has been updated, this is a pure internal refactor, not interfaces update so I don't think that a documentation update it's required

@lorenzoronzani lorenzoronzani requested a review from a team as a code owner April 29, 2026 07:20
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Apr 29, 2026 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@lorenzoronzani
Copy link
Copy Markdown
Author

lorenzoronzani commented Apr 29, 2026

/easycla

@lorenzoronzani lorenzoronzani force-pushed the refactor/http-exporter-logic branch from 2a75552 to 4405d3e Compare April 29, 2026 08:11
DylanRussell
DylanRussell reviewed Apr 29, 2026
View reviewed changes
Comment thread ...etry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py Outdated
Comment thread ...etry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py Outdated
Comment thread ...xporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py Outdated
herin049
herin049 requested changes Apr 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@herin049 herin049 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would recommend reconsidering your approach here. I'm generally not a huge fan of having functions with a huge number of arguments. I'd generally be more in favor of creating a common OTLPHttpClient, as is done in this PR

@github-project-automation github-project-automation Bot moved this to Reviewed PRs that need fixes in Python PR digest Apr 30, 2026
@lorenzoronzani
Copy link
Copy Markdown
Author

lorenzoronzani commented Apr 30, 2026

I would recommend reconsidering your approach here. I'm generally not a huge fan of having functions with a huge number of arguments. I'd generally be more in favor of creating a common OTLPHttpClient, as is done in this PR

I was thinking the same but looking around I thought that the implemented method was the one expected. I can convert with a common client class.

Thanks for the review

@lorenzoronzani
Copy link
Copy Markdown
Author

lorenzoronzani commented Apr 30, 2026

I moved these 3 public constants:

DEFAULT_COMPRESSION
DEFAULT_ENDPOINT
DEFAULT_TIMEOUT

from each exporter (Log, Metric, Trace) into the common one but this uv run tox -e public-symbols-check check now it's failing.

Q: What is the right process in this cases?

@lorenzoronzani lorenzoronzani requested a review from herin049 April 30, 2026 12:48
DylanRussell
DylanRussell reviewed Apr 30, 2026
View reviewed changes
Comment thread ...xporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py
break
return LogRecordExportResult.FAILURE
return (
LogRecordExportResult.SUCCESS
Copy link
Copy Markdown
Contributor

@DylanRussell DylanRussell Apr 30, 2026

Choose a reason for hiding this comment

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

can this Enum be part of _SignalConfig and returned directly from OTLPHttpClient

Copy link
Copy Markdown
Author

@lorenzoronzani lorenzoronzani May 4, 2026

Choose a reason for hiding this comment

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

Could be but I don't like to much that the parent needs to know children. This because you need to specify a list of types but what about in the future I want to add a new type of exporter? I should edit also the parent class.

What do you think?

DylanRussell
DylanRussell reviewed Apr 30, 2026
View reviewed changes
Comment thread CHANGELOG.md Outdated
@aabmass
Copy link
Copy Markdown
Member

aabmass commented May 11, 2026

I would recommend reconsidering your approach here. I'm generally not a huge fan of having functions with a huge number of arguments. I'd generally be more in favor of creating a common OTLPHttpClient, as is done in this PR

@herin049 @lorenzoronzani I want to make sure we're not duplicating effort. It looks like you closed #5051, was that in favor of this PR?

@herin049
Copy link
Copy Markdown
Contributor

herin049 commented May 11, 2026 •
edited
Loading

I would recommend reconsidering your approach here. I'm generally not a huge fan of having functions with a huge number of arguments. I'd generally be more in favor of creating a common OTLPHttpClient, as is done in this PR

@herin049 @lorenzoronzani I want to make sure we're not duplicating effort. It looks like you closed #5051, was that in favor of this PR?

@aabmass I actually closed out that PR to switch to #5194, given that this sounds like the direction that the SIG wants to go. It'd be better if this functionality is broken off into a separate package in order to support the work in #1003. I would suggest we close this PR or mark it as draft for now and see if we need to re-open it later.

@herin049
Copy link
Copy Markdown
Contributor

herin049 commented May 11, 2026

I would recommend reconsidering your approach here. I'm generally not a huge fan of having functions with a huge number of arguments. I'd generally be more in favor of creating a common OTLPHttpClient, as is done in this PR

@herin049 @lorenzoronzani I want to make sure we're not duplicating effort. It looks like you closed #5051, was that in favor of this PR?

Assuming we're OK with the approach in #5194, then it will be a pre-requisite to implementing the final OTLP JSON exporter package, and can also be used to simultaneously close #2990.

@lorenzoronzani
Copy link
Copy Markdown
Author

lorenzoronzani commented May 12, 2026

I would recommend reconsidering your approach here. I'm generally not a huge fan of having functions with a huge number of arguments. I'd generally be more in favor of creating a common OTLPHttpClient, as is done in this PR

@herin049 @lorenzoronzani I want to make sure we're not duplicating effort. It looks like you closed #5051, was that in favor of this PR?

Assuming we're OK with the approach in #5194, then it will be a pre-requisite to implementing the final OTLP JSON exporter package, and can also be used to simultaneously close #2990.

@herin049 @aabmass thank for the review.
Please let me know if I can help in some ways

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@herin049 herin049 Awaiting requested review from herin049

@DylanRussell DylanRussell Awaiting requested review from DylanRussell

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

Python PR digest
Status: Reviewed PRs that need fixes

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Abstract out OTLP http exporter logic

4 participants

@lorenzoronzani @aabmass @herin049 @DylanRussell