Skip to content

Conversation

@seilc1
Copy link

@seilc1 seilc1 commented May 20, 2025

Allows injecting custom TcpClient for monitoring and logging

Copy link
Collaborator

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

custom TcpClient for monitoring and logging

Seems reasonable to me.


A different approach -- more or less a quite big rewrite -- would be incorporate logging, tracing, and metrics into the lib. But that's a lot of work, and older target frameworks won't profit from that change. So your approach seems to be the better way here.

@seilc1 seilc1 force-pushed the feature/make-tcp-client-injectable branch from 6b30500 to ba1caa5 Compare May 20, 2025 14:15
@seilc1
Copy link
Author

seilc1 commented May 20, 2025

custom TcpClient for monitoring and logging

Seems reasonable to me.

A different approach -- more or less a quite big rewrite -- would be incorporate logging, tracing, and metrics into the lib. But that's a lot of work, and older target frameworks won't profit from that change. So your approach seems to be the better way here.

Besides being a bigger rewrite it's also a question of what's needed. I'm not sure if "my" need for logging/metrics would fit a general need or implementation style. So I think it's better to just provide the "possibility".

@seilc1 seilc1 requested a review from gfoidl May 22, 2025 12:22
Copy link
Collaborator

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

LGTM -- modulo one comment.

I'd like to hear what the others think about this.

Allows injecting custom TcpClient for monitoring and logging
@seilc1 seilc1 force-pushed the feature/make-tcp-client-injectable branch from ba1caa5 to 34b5f6e Compare May 22, 2025 13:09
@gfoidl
Copy link
Collaborator

gfoidl commented May 22, 2025

Please don't force push a branch when a PR is open, that makes reviewing harder, as the last diff disappears.
Just push the normal commit to the branch.

When merging the PR, then (IMO ideally) a squash-merge is done to get the linear history in the main-branch again.

@seilc1
Copy link
Author

seilc1 commented Jun 2, 2025

@gfoidl : any idea when your peers will react?

@mycroes
Copy link
Member

mycroes commented Jun 2, 2025

@gfoidl : any idea when your peers will react?

Sorry for my late response, awfully busy lately... This seems to be fine, I need to fix the workflows though so GH can also verify it builds. I'll try to get this fixed ASAP.

@seilc1
Copy link
Author

seilc1 commented Jun 2, 2025

@gfoidl : any idea when your peers will react?

Sorry for my late response, awfully busy lately... This seems to be fine, I need to fix the workflows though so GH can also verify it builds. I'll try to get this fixed ASAP.

Can I support somhow?

@S7NetPlus S7NetPlus deleted a comment from ynioba Jun 2, 2025
@mycroes
Copy link
Member

mycroes commented Jun 2, 2025

@gfoidl : any idea when your peers will react?

Sorry for my late response, awfully busy lately... This seems to be fine, I need to fix the workflows though so GH can also verify it builds. I'll try to get this fixed ASAP.

Can I support somhow?

See PR #557. Unfortunately the unit tests don't run reliably, I'm not 100% sure of the cause, but it might be that the tests execute concurrently thus causing the failures. Feel free to chime in.

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.

3 participants