Skip to content

Making GrpcStreamRetrier ID configurable #492

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Myllyenko
Copy link

Currently, GrpcStreamRetrier and its subclasses use autogenerated ID to identify themselves in logs. Such approach highly complicates correlating YDB SDK and application logs. To simplify logs correlation, in this PR the ID is being made configurable, so a user can provide his or her value upon GrpcStreamRetrier creation.

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2025

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 58.12%. Comparing base (c91ad03) to head (337b12f).

Files with missing lines Patch % Lines
...n/java/tech/ydb/topic/settings/ReaderSettings.java 60.00% 2 Missing ⚠️
...n/java/tech/ydb/topic/settings/WriterSettings.java 60.00% 2 Missing ⚠️
...in/java/tech/ydb/topic/impl/GrpcStreamRetrier.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #492      +/-   ##
============================================
- Coverage     58.15%   58.12%   -0.04%     
  Complexity     2183     2183              
============================================
  Files           325      325              
  Lines         13216    13226      +10     
  Branches       1330     1331       +1     
============================================
+ Hits           7686     7687       +1     
- Misses         4907     4914       +7     
- Partials        623      625       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alex268 alex268 requested review from pnv1 and alex268 May 26, 2025 08:36
@Myllyenko Myllyenko force-pushed the making-grpc-retrier-id-configurable branch from 23d5b5e to 9856a4b Compare May 26, 2025 15:43
@Myllyenko Myllyenko force-pushed the making-grpc-retrier-id-configurable branch from 9856a4b to 337b12f Compare May 26, 2025 15:44
@alex268 alex268 self-requested a review May 27, 2025 08:22
*
* @return this builder
*/
public Builder setDebugId(String debugId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

LogId looks better IMO, more specific.
Or LogPrefix to be even more specific

Copy link
Author

Choose a reason for hiding this comment

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

Just to be sure: are you sure that this ID won't be useful for other observability purposes?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. That identifier was created and is used for debug purpose only

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.

4 participants