chore(api): Add context information to analysis logs: BED-7265#2316
chore(api): Add context information to analysis logs: BED-7265#2316StephenHinck merged 25 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMoved three AD preprocessing steps into the AD post-processing pipeline, changed DeleteTransitEdges to accept a single Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ADPost as AD.Post
participant Preproc as PreprocessingFns
participant DeleteEdges as DeleteTransitEdges
participant PostSteps as Post*Steps
participant DB
Client->>ADPost: Trigger Post(ctx,...)
ADPost->>Preproc: FixWellKnownNodeTypes -> RunDomainAssociations -> LinkWellKnownNodes
Preproc-->>ADPost: return (stats) / error
ADPost->>DeleteEdges: DeleteTransitEdges(ctx, db, baseKinds, graph.Kinds(...))
DeleteEdges->>DB: remove transit edges
DeleteEdges-->>ADPost: edge-deletion stats
ADPost->>PostSteps: FetchLocalGroupData -> PostDCSync -> PostProtectAdminGroups -> ... -> PostNTLM
PostSteps->>DB: read/write as needed
PostSteps-->>ADPost: per-step stats / error
ADPost->>ADPost: merge per-step stats
ADPost-->>Client: return aggregated stats or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
| aggregateStats := analysis.NewAtomicPostProcessingStats() | ||
|
|
||
| if deleteTransitEdgesStats, err := analysis.DeleteTransitEdges(ctx, db, graph.Kinds{ad.Entity, azure.Entity}, ad.PostProcessedRelationships()...); err != nil { | ||
| if err := adAnalysis.FixWellKnownNodeTypes(ctx, db); err != nil { |
There was a problem hiding this comment.
These are post-processing steps that were never officially moved into the ad.Post function appropriately. Moving for consistency.
| tieringEnabled = appcfg.GetTieringEnabled(ctx, db) | ||
| ) | ||
|
|
||
| if err := adAnalysis.FixWellKnownNodeTypes(ctx, graphDB); err != nil { |
There was a problem hiding this comment.
These are post-processing steps that were never officially moved into the ad.Post function appropriately. Moving for consistency.
| defer measure.ContextMeasure( | ||
| ctx, | ||
| slog.LevelInfo, | ||
| operationName, |
There was a problem hiding this comment.
Typically, it's advised not to pass in arguments purely for logging purposes, it complicates the function with pretty small gain. Is it possible to wrap this at the call sites instead or hardcode it here?
There was a problem hiding this comment.
How do you like this instead? Small change to the function signature to take a Kinds type, rather than []Kind (which are functionally equivalent in dawgs), then built the "operationName" for use in logging, rather than passing it through the function signature.
coderabbitai
Bot
added
the
api
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/api/src/services/dataquality/dataquality.go`:
- Line 28: The deferred measurement logs currently use success-only wording like
"Successfully Completed …" which is misleading because the defer runs on all
exits (including errors); update the deferred completion message(s) in the
measurement-related defer blocks to neutral wording (e.g., "Completed …" or
"Measurement finished: …") so it doesn't imply success, and change every
occurrence referenced (the defer in the dataquality measurement setup and the
similar defer block between lines 41–55) to use the neutral message; keep the
rest of the defer behavior and any timing/metric recording intact.
In `@packages/go/analysis/post.go`:
- Around line 226-233: The logging attribute incorrectly lists
attr.Function("FetchComputersWithURA") inside the measure.ContextMeasure call
used for the "Finished deleting orphaned nodes" block; replace that string with
the actual enclosing function name (e.g., attr.Function("DeleteOrphanedNodes")
or whatever the real function is) so the function attribute on
measure.ContextMeasure matches the real function that calls it (update the
attr.Function value where measure.ContextMeasure(...) is deferred).
🧹 Nitpick comments (2)
cmd/api/src/daemons/datapipe/pipeline.go (1)
264-269: Missingattr.Scopeattribute for consistency.The
measure.LogAndMeasurecall at line 238 includesattr.Scope("summary"), but thisslog.InfoContextcall omits theScopeattribute. For consistency in log analysis and filtering, consider adding the same scope.Suggested fix
} else { slog.InfoContext( ctx, "Cache successfully reset by datapipe daemon", attr.Namespace("analysis"), attr.Function("Analyze"), + attr.Scope("summary"), ) }cmd/api/src/analysis/ad/post.go (1)
50-50: Unnecessary double parentheses.The inner parentheses around
ad.PostProcessedRelationships()are redundant.✨ Suggested fix
- } else if deleteTransitEdgesStats, err := analysis.DeleteTransitEdges(ctx, db, graph.Kinds{ad.Entity, azure.Entity}, graph.Kinds((ad.PostProcessedRelationships()))); err != nil { + } else if deleteTransitEdgesStats, err := analysis.DeleteTransitEdges(ctx, db, graph.Kinds{ad.Entity, azure.Entity}, graph.Kinds(ad.PostProcessedRelationships())); err != nil {
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/go/analysis/post.go (1)
166-181: 🛠️ Refactor suggestion | 🟠 MajorGroup and hoist variable initializations into a single
varblock.
operationNameis initialized separately from the existing var block. Please group these initializations and hoist them to the top of the function to comply with the Go guidelines.Proposed refactor
func DeleteTransitEdges(ctx context.Context, db graph.Database, baseKinds graph.Kinds, targetRelationships graph.Kinds) (*AtomicPostProcessingStats, error) { - operationName := fmt.Sprintf("Delete %v post-processed relationships", strings.Join(targetRelationships.Strings(), ", ")) - - defer measure.ContextMeasure( - ctx, - slog.LevelInfo, - operationName, - attr.Namespace("analysis"), - attr.Function("DeleteTransitEdges"), - attr.Scope("process"), - )() - var ( + operationName = fmt.Sprintf("Delete %v post-processed relationships", strings.Join(targetRelationships.Strings(), ", ")) relationshipIDs []graph.ID - stats = NewAtomicPostProcessingStats() + stats = NewAtomicPostProcessingStats() ) + + defer measure.ContextMeasure( + ctx, + slog.LevelInfo, + operationName, + attr.Namespace("analysis"), + attr.Function("DeleteTransitEdges"), + attr.Scope("process"), + )()As per coding guidelines: “Group variable initializations in a
var ( ... )block and hoist them to the top of the function when possible.”
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
3 participants
Description
Added contextual information to logs identified as common during analysis runs to provide improved context during log review and investigation of issues. Currently, those logs may prevent a user or administrator from understanding where within the process a certain log applies or is related to other steps in the process.
Motivation and Context
Resolves BED-7265
Why is this change required? What problem does it solve?
How Has This Been Tested?
Validated locally through multiple analysis runs with test data.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit