feat: Feature view request debug logging#375
Open
vanitabhagwat wants to merge 17 commits into
Open
Conversation
Implement config parsing for three environment variables: - DEBUG_LOGGING: enables debug logging (boolean) - FEAST_LOG_SAMPLE_RATE: sampling rate for debug requests (0.0-1.0) - FEAST_LOG_ENTITY_KEY_SALT: salt for hashing entity keys Config struct provides these fields with sensible defaults and error fallbacks. Covers Plan B (config + sampling) of the debug logging approach. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…aveat
Finding 1 — TestHashEntityKeys_OneHashPerJoinKeySortedByName now verifies
actual sort order by computing expected hashes for individual keys and
asserting they appear in the correct positions in the two-entity result.
This ensures a regression that removes sort.Strings(names) will be caught.
Finding 2 — Added doc comment explaining that proto.MarshalOptions{Deterministic: true}
only guarantees byte-stability within a single binary/process version, not
across builds/deploys. Flagged that this is currently safe because RepeatedValue
and Value have no map fields; must revisit if schema changes add maps.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implement ClassifyError to extract gRPC status codes from errors for debug logging. Reuses the existing errors.GrpcFromError classification to maintain consistency with framework error handling. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…andlers Mirrors the http_server.go wiring: flags requests via debuglogging.RequestFlaggedGRPC, stores a debugLogCfg on the server, and emits a structured debug log line via EmitDebugRequestLog/ EmitDebugRequestLogRange on both success and error paths. Guards s.metricsCtx.Project/OnlineStore access behind a nil check, since NewMetricsContext returns nil when the metrics client or repo config is unset (as in the grpc_server_test.go test harness) — a direct field access there panics with a nil pointer dereference, which TestGetOnlineFeaturesSqlite caught.
getOnlineFeatures and getOnlineFeaturesRange in http_server.go read s.metricsCtx.Project/OnlineStore directly when building EmitDebugRequestLog(Range) calls. NewMetricsContext returns nil when no metrics client/config is configured, so this panics with a nil-pointer dereference. Mirrors the identical fix already applied to grpc_server.go (845ae06): introduce nil-guarded local debugProject/debugOnlineStore variables instead of dereferencing s.metricsCtx directly. Adds TestGetOnlineFeaturesHttpSqlite, a regression test exercising getOnlineFeatures via GetHttpServer's nil-metrics-client configuration (the same seam TestGetOnlineFeaturesSqlite uses for the gRPC side). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Without a salt, the entity-key hash used in debug logging is just sha256(value), which is reversible/brute-forceable for the small, bounded ID spaces Feast entity keys typically use. This satisfies the letter of "never plaintext" but defeats the intent of never exposing which entity was queried. Log a WARNING at startup when the salt is unset so the risk is visible instead of silent; hash behavior is unchanged.
Drop the entity_key_hash field from the structured debug log entirely, along with the now-dead supporting machinery: the HashEntityKeys function, the Config.Salt field, the FEAST_LOG_ENTITY_KEY_SALT env var, and the startup salt-warning. Entity keys are now never logged in any form (not even hashed), which over-satisfies the "no plaintext entity keys" requirement and removes the reversibility risk that the batch-level salted hash carried for low-cardinality IDs. Also removes the unconditional startup WARN this machinery required. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… test Finding #2: features_returned_count previously used len(featureVectors), which includes the entity/join-key columns that precede the feature columns in the response (per the resp.Results[len(entities):] convention), so it was inflated relative to features_requested. Subtract the entity-key count (clamped at 0 for the error path) so the two fields are directly comparable. Finding #4: the explicit-null error_type assertion used assert.Nil on the decoded map value, which passes whether the key is present-with-null or absent. Use the comma-ok form to require the key is actually emitted, pinning the "explicit JSON null, not omitted" behavior. Adds regression tests for the entity-column subtraction and the error-path clamp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vanitabhagwat
force-pushed
the
feature-view-request-debug-logging
branch
from
July 9, 2026 21:34
a614da8 to
9538954
Compare
vanitabhagwat
changed the title
These are local design/planning docs, not part of the codebase; ignoring them prevents accidental inclusion in commits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
piket
reviewed
Jul 16, 2026
Comment on lines
+181
to
+185
| // rangeFeatureVectors includes one column per entity join key ahead of | ||
| // the feature columns, so subtract entityKeyCount to make this directly | ||
| // comparable to featuresRequested (clamp at 0 for the error path where | ||
| // rangeFeatureVectors is nil). | ||
| FeaturesReturnedCount: max(0, len(rangeFeatureVectors)-entityKeyCount), |
Collaborator
There was a problem hiding this comment.
I don't think this is correct. I believe entities are not included in the range feature vectors, but are added on later as a separate node. Please double check this.
piket
reviewed
Jul 16, 2026
…ed for FeatureService Range feature vectors never include entity-key columns (unlike the non-range path which prepends them via EntitiesToFeatureVectors), so remove the entityKeyCount subtraction from EmitDebugRequestLogRange. For FeatureService requests FeaturesRefs is nil, causing features_requested to log as 0. Fall back to the returned-vector count on the success path so the field reflects actual feature count for both explicit-ref and FeatureService requests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Adds structured, per-request debug logging to the Go feature server for getOnlineFeatures() / getOnlineFeaturesRange() on both HTTP and gRPC transports. The goal is per-request debugging and cross-system correlation (custom metrics
→ APM → Splunk) via trace_id, complementing the per-feature-view metrics and APM sub-spans added previously.
Two independent trigger mechanisms are provided; either one alone is sufficient, so operators can choose:
Each sampled/flagged request emits exactly one structured JSON log line containing (as available):
The event is emitted on both the success and error paths. Requests that are neither flagged nor sampled emit nothing and incur negligible overhead (the counting work is gated behind the emit decision).
Which issue(s) this PR fixes:
Misc