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.

Sign up for GitHub

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

Jump to bottom

Remove circular dependency between default otel and connector #9095

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Remove circular dependency between default otel and connector #9095

merged 2 commits into from
Dec 14, 2023

Conversation

bogdandrutu
Copy link
Member

No description provided.

@bogdandrutu bogdandrutu requested review from a team and codeboten December 13, 2023 00:27
@bogdandrutu bogdandrutu force-pushed the rmcircular branch 2 times, most recently from 4dace2c to c7dbbfa Compare December 13, 2023 00:28
@codecov Codecov
Copy link

codecov bot commented Dec 13, 2023 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (f08baa9) 91.53% compared to head (65e88f4) 91.52%.

Files Patch % Lines
connector/logs_router.go 97.05% 1 Missing ⚠️
connector/metrics_router.go 97.05% 1 Missing ⚠️
connector/traces_router.go 97.05% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9095      +/-   ##
==========================================
- Coverage   91.53%   91.52%   -0.02%     
==========================================
  Files         316      319       +3     
  Lines       17215    17218       +3     
==========================================
  Hits        15758    15758              
- Misses       1160     1163       +3     
  Partials      297      297

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

@bogdandrutu bogdandrutu force-pushed the rmcircular branch 2 times, most recently from fd26316 to f1f75cb Compare December 13, 2023 00:56
dmitryax
dmitryax reviewed Dec 13, 2023
View reviewed changes
connector/metrics_router.go Outdated Show resolved Hide resolved
connector/logs_router_test.go Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu force-pushed the rmcircular branch 2 times, most recently from 027d5dd to 204c37c Compare December 13, 2023 07:20
djaglowski
djaglowski reviewed Dec 13, 2023
View reviewed changes
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

I'm working on something closely related and would like some time to reconcile possible changes with this PR. I may have some suggestions based on this perspective. I will follow up here today or tomorrow.

djaglowski
djaglowski approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

This looks good to me. The changes I am looking at elsewhere would fit into this just as well either way.

codeboten
codeboten reviewed Dec 13, 2023
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I believe this change should be done by first deprecating the original interfaces no? https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#breaking-changes

@djaglowski
Copy link
Member

Hmm, the intention isn't to remove the interfaces, but changing them does break them. Do we deprecate, then remove the deprecation while making the change?

@codeboten
Copy link
Contributor

codeboten commented Dec 13, 2023 •
edited
Loading

Hmm, the intention isn't to remove the interfaces, but changing them does break them. Do we deprecate, then remove the deprecation while making the change?

I believe this was done in the past by providing a temporary interface name, that will then be renamed again. ie

v0.92

// deprecated: use FooWithSomething
type Foo

type FooWithSomething

v0.93


type Foo

// deprecated: use Foo
type FooWithSomething

@bogdandrutu

This comment was marked as resolved.

Sign in to view
@bogdandrutu bogdandrutu force-pushed the rmcircular branch 2 times, most recently from deb315f to 4a3d296 Compare December 14, 2023 07:27
@bogdandrutu bogdandrutu merged commit edf7aca into open-telemetry:main Dec 14, 2023
32 checks passed
@bogdandrutu bogdandrutu deleted the rmcircular branch December 14, 2023 07:50
@github-actions github-actions bot added this to the next release milestone Dec 14, 2023
sokoide pushed a commit to sokoide/opentelemetry-collector that referenced this pull request Dec 18, 2023
@bogdandrutu @sokoide
Remove circular dependency between default otel and connector (open-t…
d45ce11 
…elemetry#9095)

Signed-off-by: Bogdan Drutu <[email protected]>
@atoulme atoulme mentioned this pull request Mar 17, 2024
Merged
dmitryax pushed a commit that referenced this pull request Mar 20, 2024
@atoulme
[connector] Remove deprecated interfaces LogsRouter, MetricsRouter an…
4f6b7df 
…d TracesRouter (#9780)

**Description:**
Remove deprecated interfaces LogsRouter, MetricsRouter and TracesRouter.

**Link to tracking Issue:**
Follow up to #9095
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten left review comments

@dmitryax dmitryax dmitryax approved these changes

@djaglowski djaglowski djaglowski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.92.0
Development

Successfully merging this pull request may close these issues.
4 participants
@bogdandrutu @djaglowski @codeboten @dmitryax