Skip to content

Conversation

@qrort
Copy link
Collaborator

@qrort qrort commented Dec 19, 2025

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

node_hint_miss metric reports node_hint info for all requests, even when preferred session_id equals actual session_id

What is the new behavior?

added "hit" label to metrics to distinguish between hits and misses

Other information

@github-actions
Copy link

summary

Inferred base version: v3.124.0
Suggested version: v3.124.1

@github-actions github-actions bot removed the SLO label Dec 19, 2025
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2025

Codecov Report

❌ Patch coverage is 11.11111% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.08%. Comparing base (d9d08f6) to head (aad52f6).

Files with missing lines Patch % Lines
metrics/query.go 14.28% 6 Missing ⚠️
metrics/table.go 14.28% 6 Missing ⚠️
metrics/node_id.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1990      +/-   ##
==========================================
+ Coverage   73.73%   74.08%   +0.35%     
==========================================
  Files         396      396              
  Lines       41251    34645    -6606     
==========================================
- Hits        30415    25667    -4748     
+ Misses       9721     7852    -1869     
- Partials     1115     1126      +11     
Flag Coverage Δ
experiment 73.74% <11.11%> (+0.35%) ⬆️
go-1.21.x 72.93% <11.11%> (+3.86%) ⬆️
go-1.25.x 74.06% <11.11%> (+0.37%) ⬆️
integration 55.07% <11.11%> (+0.87%) ⬆️
macOS 47.70% <0.00%> (-0.07%) ⬇️
ubuntu 74.08% <11.11%> (+0.37%) ⬆️
unit 47.71% <0.00%> (-0.06%) ⬇️
windows 47.69% <0.00%> (-0.04%) ⬇️
ydb-24.4 54.26% <11.11%> (+0.89%) ⬆️
ydb-25.2 55.01% <11.11%> (+1.11%) ⬆️
ydb-latest 54.59% <11.11%> (+1.06%) ⬆️
ydb-nightly 73.74% <11.11%> (+0.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@qrort qrort force-pushed the node-hint-metric-improve branch from f35e0df to f0557dd Compare December 19, 2025 12:52
@qrort qrort added the SLO label Dec 19, 2025
@github-actions github-actions bot removed the SLO label Dec 19, 2025
@github-actions
Copy link

github-actions bot commented Dec 19, 2025

🌋 SLO Test Results

Status: 🟡 6 workloads tested • 6 workloads with warnings

Workload Metrics Regressions Improvements Links
🟡 native-table 8 0 0 ReportCheck
🟡 native-bulk-upsert 8 0 0 ReportCheck
🟡 native-query 8 0 0 ReportCheck
🟡 native-table-over-query-service 8 0 1 ReportCheck
🟡 database-sql-query 8 0 0 ReportCheck
🟡 database-sql-table 8 0 0 ReportCheck

Generated by ydb-slo-action

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves the node_hint metrics by renaming the metrics from node_hint_miss to node_hint and adding a "hit" label to distinguish between cache hits and misses, addressing the issue where the previous implementation reported all requests as misses even when the preferred session ID matched the actual session ID.

  • Renamed metrics from *_node_hint_miss to *_node_hint with an added "hit" label
  • Introduced string constants trueStr and falseStr for consistent labeling
  • Implemented hit/miss detection logic based on comparison of preferred and actual node IDs

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
metrics/query.go Added constants and updated node hint metric with hit detection logic for query pool
metrics/table.go Updated node hint metric with hit detection logic for table pool, using constants from query.go
CHANGELOG.md Documented the breaking change in metric naming and added hit label feature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI commented Dec 19, 2025

@asmyasnikov I've opened a new pull request, #1991, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Dec 19, 2025

@asmyasnikov I've opened a new pull request, #1992, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Dec 19, 2025

@asmyasnikov I've opened a new pull request, #1993, to work on those changes. Once the pull request is ready, I'll request review from you.

@qrort qrort force-pushed the node-hint-metric-improve branch from 049723b to aad52f6 Compare December 19, 2025 15:23
@qrort qrort added the SLO label Dec 19, 2025
@github-actions github-actions bot removed the SLO label Dec 19, 2025
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