feat(tree): emit telemetry when healing unresolvable identifiers on decode - #27756
feat(tree): emit telemetry when healing unresolvable identifiers on decode#27756CraigMacomber wants to merge 5 commits into
Conversation
…ecode When healUnresolvableIdentifiersOnDecode is enabled, SharedTree now records a HealUnresolvableIdentifierOnDecode telemetry event (at LogLevel.info) each time a non-final identifier is healed during summary load, so affected documents can be observed. Adds an optional logger to IdentifierHealingConfig, threads the shared object's logger through, documents the behavior, and adds a changeset.
github-actions
Bot
added
area: tools
area: dds
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (76 lines, 5 files), I've queued these reviewers:
How this works
|
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
| category: "generic", | ||
| eventName: "HealUnresolvableIdentifierOnDecode", | ||
| }, | ||
| LogLevel.info, |
There was a problem hiding this comment.
I do think this should be essential. Rationale -the other side of the if (healing !== undefined) check throws an error which we will for sure log at essential level (probably with multiple events as it propagates).
So we should match both branches to be able to definitively monitor the fix. I'm not worried about the overall volume since there's natural attrition due to not using the document or subsequent summaries persisting the fixed ID.
| const healed = uuidV5(`${healing.sharedObjectId}|${id}`, healingNamespace); | ||
| healing.logger?.send( | ||
| { | ||
| category: "generic", |
There was a problem hiding this comment.
If you switch the type of logger to be ITelemetryLoggerExt and use sendEvent or sendTelemetryEvent or whatever then you won't need this category ("generic" is default). That's the interface we typically use for intrumenting. (The base logger is what is exposed across seams for passing logs around)
There was a problem hiding this comment.
There are reasons not to do this, mostly related to potential future refactors and shared-tree internals which can accept loggers directly from users which might not implement ITelemetryLoggerExt.
Specifically if this code becomes reachable from IndependentViews which don't have a main fluid logger, we could run into issues.
| ); | ||
| }); | ||
|
|
||
| it("does not log when the ID is resolvable", () => { |
There was a problem hiding this comment.
Want to add a test case for not logging when it throws (not resolvable and not healed)
There was a problem hiding this comment.
That case is only possible when forceDecodeEncodedIdWithoutSession is passed no IdentifierHealingConfig, and since the logger is part of IdentifierHealingConfig, that would involve testing that an API which is not passed the longer does not log anything.
I don't think thats helpful for two reasons:
- If would actually be ok if we logged telemetry in the error case.
- Its impossible that it would log anything as it has no access to the logger.
We could add a higher level integration case for that case, but that would amount to testing that loading a shared tree with an invalid summary throws a data corruption error (which would be logged), and I hope we have testing for that somewhere else. We do have testing in this file that an unrecovered ID does throw.
|
wait for #27824 to merge |
Bundle size comparisonBase commit: Notable changes
Per-bundle deltas
|
Description
When
SharedTreeOptionsBeta.healUnresolvableIdentifiersOnDecodeis enabled, SharedTree now records aHealUnresolvableIdentifierOnDecodetelemetry event (atLogLevel.info) each time a non-final identifier is healed while loading a summary. This lets applications relying on the healing workaround observe which documents actually required healing.This is a follow-up to the identifier-healing work: the heal and throw paths only exist because of a prior bug where non-finalized identifiers could be written into summaries. Instrumenting the recovery path makes those occurrences visible in the wild. The error/throw path is intentionally left un-instrumented — the thrown exception already surfaces via the application's own error telemetry and must not be silently swallowed.
Changes:
loggerto the internalIdentifierHealingConfig;forceDecodeEncodedIdWithoutSessionsends the event directly vialogger?.send(..., LogLevel.essential).SharedTreeKernelandSharedTreeCore.healUnresolvableIdentifiersOnDecode.Because it changes observable behavior for consumers who opted into the healing feature (they now get telemetry), this ships with a changeset.
Reviewer Guidance
The review process is outlined on this wiki page.
IdentifierHealingConfig(recovery path only) rather than being threaded independently; the throw path is deliberately not instrumented. Confirm that split is acceptable.