Skip to content

CBG-4497 centralize blip correlation ids #7498

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

Merged
merged 5 commits into from
Apr 29, 2025
Merged

CBG-4497 centralize blip correlation ids #7498

merged 5 commits into from
Apr 29, 2025

Conversation

torcolvin
Copy link
Collaborator

@torcolvin torcolvin commented Apr 24, 2025

CBG-4497 centralize blip correlation ids

Force assignment of blip correlation IDs to match the blip context for _blipsync and ISGR. The important change is in NewSGBlipContextWithProtocols to have bc.Logger = defaultBlipLogger(ctx) use the right correlation id.

ISGR and _blipsync did reset the context on BlipSyncContext after its construction, but this seems confusing and error prone, so I just forced setting the correlation id in NewBlipSyncContext constructor. I could leave the this part of the code alone and just do the change in NewSGBlipContextWithProtocols

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

@torcolvin torcolvin requested a review from bbrks April 24, 2025 15:03
Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

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

This has subtly changed the format of BLIP context IDs which is probably undesirable.
I think it's just a case of passing the context ID through FormatBlipContextID()?

Otherwise looks OK to me

Compare before:

2025-04-25T13:04:59.891+01:00 [INF] HTTP: c:#005 db:db1 GET /db1/_blipsync (as <ud>demo</ud>)
2025-04-25T13:04:59.891+01:00 [INF] HTTP+: c:[1e02c999] db:db1 #005:     --> 101 [1e02c999] Upgraded to WebSocket protocol BLIP_3+CBMobile_2 (as <ud>demo</ud>)  (0.0 ms)
2025-04-25T13:04:59.891+01:00 [INF] WS: c:#005 db:db1 Start BLIP/Websocket handler
2025-04-25T13:04:59.892+01:00 [INF] SyncMsg: c:[1e02c999] db:db1 col:_default #1: Type:changes #Changes:200
2025-04-25T13:04:59.950+01:00 [DBG] SyncMsg+: c:[1e02c999] db:db1 #1: Type:changes   --> OK Time:58.634333ms

After:

2025-04-25T13:08:14.918+01:00 [INF] HTTP: c:#007 db:db1 GET /db1/_blipsync (as <ud>demo</ud>)
2025-04-25T13:08:14.918+01:00 [INF] HTTP+: c:[47f59398] db:db1 #007:     --> 101 [47f59398] Upgraded to WebSocket protocol BLIP_3+CBMobile_2 (as <ud>demo</ud>)  (0.0 ms)
2025-04-25T13:08:14.918+01:00 [INF] WS: c:47f59398 db:db1 Start BLIP/Websocket handler
2025-04-25T13:08:14.918+01:00 [INF] SyncMsg: c:47f59398 db:db1 col:_default #1: Type:changes #Changes:200
2025-04-25T13:08:14.976+01:00 [DBG] SyncMsg+: c:47f59398 db:db1 #1: Type:changes   --> OK Time:57.73725ms

Note: /ks/_blipsync endpoint and active replicator have different
formats
@torcolvin torcolvin requested a review from bbrks April 28, 2025 12:46
Copy link
Member

@bbrks bbrks left a comment

Choose a reason for hiding this comment

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

Looks good sans honk

@bbrks bbrks assigned torcolvin and unassigned bbrks Apr 28, 2025
Co-authored-by: Ben Brooks <[email protected]>
db/blip.go Outdated
@@ -12,6 +12,7 @@ package db

import (
"context"
"fmt"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"fmt"

@torcolvin torcolvin merged commit a27fe98 into main Apr 29, 2025
44 checks passed
@torcolvin torcolvin deleted the CBG-4497 branch April 29, 2025 00:46
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.

2 participants