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

[receiver/statsd] Refactor of hardcoded tests #24832

Conversation

kang-makes
Copy link
Contributor

@kang-makes kang-makes commented Aug 3, 2023

Description:
This is the first step to adding UDS support to the StatsD receiver.

As I started to develop it, I saw that all tests were hardcoded to UDP and using networking and there was no possibility to add a socket communication.

PR started to get huge so I split it into two, one to refactor tests and another one that adds UDS support:

  • Removed all unused references.
  • Made a Transport helper that allows centralizing all supported protocols and constants in its package.
  • Removed all hardcoded UDP protocols and generalized testing so new protocols are easy to add.

If you need a rationale about why the changes are like this, this is the next PR I am going to submit after this one is merged: kang-makes#2

That is the PR that is going to add UDS support properly.

Link to tracking Issue:

@kang-makes kang-makes requested a review from a team August 3, 2023 09:30
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

@kang-makes please rebase

# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.

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

Choose a reason for hiding this comment

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

Is there a bug that fixed by this PR? If it's just refactoring, no changelog entry is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remove it on the commit #7094a68

@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 Aug 19, 2023
@dmitryax dmitryax removed the Stale label Aug 19, 2023
@kang-makes kang-makes force-pushed the receiver/statsdreceiver/add-uds-support branch from 59faed9 to 7094a68 Compare August 21, 2023 10:52
@kang-makes
Copy link
Contributor Author

Rebased :)

@kang-makes kang-makes requested a review from dmitryax August 21, 2023 11:58
@kang-makes kang-makes force-pushed the receiver/statsdreceiver/add-uds-support branch from 7094a68 to e4f8238 Compare August 22, 2023 09:40
@kang-makes
Copy link
Contributor Author

Rebased.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

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 Sep 6, 2023
@kang-makes kang-makes force-pushed the receiver/statsdreceiver/add-uds-support branch from e4f8238 to 4fa23ed Compare September 7, 2023 13:46
@kang-makes
Copy link
Contributor Author

kang-makes commented Sep 7, 2023

Rebased again. Could you take a look @dmitryax? I am waiting for this PR to be merged so I can create a new pull request with the feature I really want to add (#21385) which code waiting here :D

@github-actions github-actions bot removed the Stale label Sep 8, 2023
@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 Sep 22, 2023
@github-actions github-actions bot removed the Stale label Sep 23, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2023

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 Oct 8, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 23, 2023
@kang-makes
Copy link
Contributor Author

@dmitryax I have just rebased it and included in this PR the support for TCP that was added by the community.

Let me know if I can do something more because I cannot reopen the PR :(

bogdandrutu pushed a commit that referenced this pull request Dec 15, 2023
**Description:** 
This is the first step to adding UDS support to the StatsD receiver.

As I started to develop it, I saw that all tests were hardcoded to UDP
and using networking and there was no possibility to add a socket
communication.

PR started to get huge so I split it into two, one to refactor tests and
another one that adds UDS support:
 * Removed all unused references.
* Made a `Transport` helper that allows centralizing all supported
protocols and constants in its package.
* Removed all hardcoded UDP protocols and generalized testing so new
protocols are easy to add.

If you need a rationale about why the changes are like this, this is the
next PR I am going to submit after this one is merged:
kang-makes#2

That is the PR that is going to add UDS support properly.

**Link to tracking Issue:**
 - #21385

**Previous closed PR:**
 - #24832
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jan 10, 2024
…28896)

**Description:** 
This is the first step to adding UDS support to the StatsD receiver.

As I started to develop it, I saw that all tests were hardcoded to UDP
and using networking and there was no possibility to add a socket
communication.

PR started to get huge so I split it into two, one to refactor tests and
another one that adds UDS support:
 * Removed all unused references.
* Made a `Transport` helper that allows centralizing all supported
protocols and constants in its package.
* Removed all hardcoded UDP protocols and generalized testing so new
protocols are easy to add.

If you need a rationale about why the changes are like this, this is the
next PR I am going to submit after this one is merged:
kang-makes#2

That is the PR that is going to add UDS support properly.

**Link to tracking Issue:**
 - open-telemetry#21385

**Previous closed PR:**
 - open-telemetry#24832
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants