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

[receiver/zipkin] move global server configuration parameters to a separate parameter #35851

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[receiver/zipkin] move global server configuration parameters to a separate parameter #35851

wants to merge 2 commits into from
+333 −74

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Oct 17, 2024 •
edited
Loading

Description

Move global server configuration parameters to a separate parameter under protocols parameter to allow future support of different protocols (like UDP), more context here

Link to tracking issue

Fixes #35730

Testing

Documentation

@github-actions github-actions bot added the receiver/zipkin Zipkin receiver label Oct 17, 2024
@odubajDT odubajDT changed the title [receiver/zipkin] move global server configuration parameters to a se… [receiver/zipkin] move global server configuration parameters to a separate parameter Oct 17, 2024
@github-actions github-actions bot requested a review from yotamloe October 17, 2024 11:30
@odubajDT odubajDT marked this pull request as ready for review October 17, 2024 13:04
@odubajDT odubajDT requested a review from a team as a code owner October 17, 2024 13:04
@odubajDT odubajDT force-pushed the refactor-zipkinreceiver branch from 19be99b to d7fe5ae Compare October 24, 2024 09:59
@odubajDT
Copy link
Contributor Author

@andrzej-stencel would you please have time for a review?

@odubajDT odubajDT force-pushed the refactor-zipkinreceiver branch from 0fc263b to ca05388 Compare November 7, 2024 12:07
andrzej-stencel
andrzej-stencel reviewed Nov 13, 2024
View reviewed changes
.chloggen/refactor-zipkinreceiver.yaml
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: breaking
Copy link
Member

@andrzej-stencel andrzej-stencel Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you explain why marking this as a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's changing the configuration of the receiver even if it's changed behind a feature gate, but the user will be affected if the feature gate will be turned on/off. Do you consider this more as a deprecation?

@odubajDT odubajDT force-pushed the refactor-zipkinreceiver branch 2 times, most recently from 5c8e10f to c476c08 Compare November 20, 2024 06:29
andrzej-stencel
andrzej-stencel reviewed Nov 27, 2024
View reviewed changes
receiver/zipkinreceiver/config.go Outdated Show resolved Hide resolved
receiver/zipkinreceiver/config.go Outdated Show resolved Hide resolved
receiver/zipkinreceiver/config.go Outdated Show resolved Hide resolved
receiver/zipkinreceiver/config.go Outdated Show resolved Hide resolved
@odubajDT odubajDT force-pushed the refactor-zipkinreceiver branch 3 times, most recently from 913feff to 6e1a767 Compare December 3, 2024 13:17
@odubajDT odubajDT force-pushed the refactor-zipkinreceiver branch from 6e1a767 to 44ff051 Compare December 4, 2024 13:48
@odubajDT odubajDT force-pushed the refactor-zipkinreceiver branch from 44ff051 to a12483d Compare December 6, 2024 07:17
@github-actions GitHub Actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 21, 2024
@andrzej-stencel
Copy link
Member

Hey @odubajDT what is the status of this PR? Is this issue resolved?

With the force pushes, it's hard for me to track the progress on the PR. Making the commit messages more informative could help here.

@github-actions github-actions bot removed the Stale label Jan 3, 2025
@odubajDT
Copy link
Contributor Author

odubajDT commented Jan 7, 2025

Hey @odubajDT what is the status of this PR? Is this issue resolved?

With the force pushes, it's hard for me to track the progress on the PR. Making the commit messages more informative could help here.

Hi, yes the issue should be resolved according to your description. I will resolve the merge conflicts and it should be ready for another round of review

@odubajDT odubajDT force-pushed the refactor-zipkinreceiver branch from a12483d to b50af84 Compare January 7, 2025 10:27
@odubajDT
go mod tidy
746832a 
Signed-off-by: odubajDT <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrzej-stencel andrzej-stencel andrzej-stencel left review comments

@crobert-1 crobert-1 Awaiting requested review from crobert-1 crobert-1 is a code owner

@MovieStoreGuy MovieStoreGuy Awaiting requested review from MovieStoreGuy MovieStoreGuy is a code owner

@yotamloe yotamloe Awaiting requested review from yotamloe

@carsonip carsonip Awaiting requested review from carsonip

@JaredTan95 JaredTan95 Awaiting requested review from JaredTan95

@lahsivjar lahsivjar Awaiting requested review from lahsivjar

At least 1 approving review is required to merge this pull request.

Assignees

@mx-psi mx-psi

Labels
exporter/elasticsearch exporter/logzio exporter/zipkin receiver/zipkin Zipkin receiver testbed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor Zipkin Receiver to include Protocols in Config
3 participants
@odubajDT @andrzej-stencel @mx-psi