Repository navigation
VCST-5337: GA reports support - #61
Conversation
Compare GA event names with EqualsIgnoreCase, as everywhere else in the module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Run the newest-bucket probes concurrently instead of nose to tail. The degree is capped because GA4 limits concurrent requests per property and a criteria naming no events can carry up to MaxEventNames of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bound an unnamed summary at 50 event names, not GA4's 500-name property cap: a summary over hundreds of names is a reporting question, not a feature read, and this is what the probe fan-out is bounded by. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alexeyshibanov
left a comment
alexeyshibanov
left a comment
There was a problem hiding this comment.
Request changes — four major findings, none of them a blocker, plus a cross-repo one that lands in the companion PR. The design is well argued and the description is unusually thorough; everything below survived being re-read against the branch.
Build is clean (0 warnings) and 64/64 tests pass.
Cross-cutting
The "source-agnostic" provider seam is not reachable from the contracts package. IAnalyticsDataSource, IAnalyticsSettingsResolver, AnalyticsDataQuery and AnalyticsDataApiSettings all live in .Data, while IAnalyticsService lives in .Core. So the replacement provider the comment invites ("a first-party event store, a BigQuery export") must reference the package that carries Google.Analytics.Data.V1Beta and Google.Apis.Auth. Two further leaks: the dimension vocabulary consumers pass (eventName, dateHour, searchTerm, itemId) is literally the GA4 apiName set, and AnalyticsDiagnosticsService — provider-agnostic in name, behind a .Core interface — depends directly on IGoogleAnalyticsReportClient, RpcException, CheckCompatibilityRequest and Google's Metadata, so swapping the data source leaves diagnostics checking Google. The customUser: prefix is not part of the leak; it is applied and stripped entirely inside the Google implementation.
The read API has no channel for "broken". IsConfiguredAsync returns false both when nothing is configured and when the resolver throws; SearchEventsAsync and GetEventSummariesAsync return an empty result for an unconfigured store, a settings failure and any data-source failure alike. The swallowing itself is declared and defensible — a GA outage should degrade a widget, not fail a page — but a consumer cannot distinguish "no events" from "misconfigured", and the only record is a server warning. The diagnostics endpoint is an out-of-band admin check, so the calling code gets nothing. A status or result envelope costs nothing on a brand-new API and is a breaking change later. Same window for CancellationToken: none of the three service interfaces or the five client members takes one, on an operation whose worst case is 1 totals read plus up to 50 probes, and adding a parameter later changes the arity compiled callers reference plus every implementer's signature.
Affects the companion PR
The dateHour timezone finding below is a defect here and a visible symptom there: vc-module-sales-rep merges these hour buckets against exact-UTC order timestamps and sorts by them, so "newest first" is wrong by the property's offset. Its GraphQL description also repeats the UTC claim.
Second cross-repo one: BuildDimensionFilter drops any filter whose Values is empty. The companion module builds its data-isolation filter from a caller-supplied organization list, so an empty list there yields a read scoped only by session_kind rather than an empty result. It is guarded three times over on that side today, so this is a defence-in-depth request rather than a live hole — but the drop is silent, and neither repository's tests can see it.
Description
The Description paragraph says "Four new store-level settings"; there are two (DataApi.PropertyId, DataApi.CacheTtlMinutes), and the PR's own New settings table and Localization section both say two — probably a leftover from before the "Obsolete settings removed" commit. The neighbouring claim in the same paragraph does check out: three public tagging settings, anonymous GET untouched.
The docs' claim that diagnostics "never reports a credential's contents — only its kind" needs narrowing or the payload needs sanitising; see the inline comment.
Verified clean
Additive throughout — no public member removed, renamed or re-signatured, and .Web is not packable so the controller's new constructor parameter reaches no consumer. Cached values are cloned on read at both levels. Diagnostics genuinely bypasses the cache. Every new public service method is virtual and every seam protected virtual. All new models are created through AbstractTypeFactory.TryCreateInstance(). platformVersion correctly stays at 3.1039.0 and matches the Platform.Core reference. All ten locale files carry both new settings, genuinely translated. No persistence, so no migration dimension. The alpha/manifest pinning form is right for the companion's prerelease.
One test-coverage note: AnalyticsFilterBuilder — which builds every dimension filter, including the ones carrying the isolation guarantee for the companion module — has no test of its own and is reached only indirectly.
|
|
||
| private static DateTime? ParseDateHour(string value) | ||
| { | ||
| return DateTime.TryParseExact(value, DateHourFormat, CultureInfo.InvariantCulture, |
There was a problem hiding this comment.
major — dateHour is in the property's timezone, and this stamps it as UTC.
AssumeUniversal | AdjustToUniversal asserts something the GA4 Data API contradicts, and the API ships the correction in the same response. From googleapis/googleapis, google/analytics/data/v1beta/data.proto:
// The property's current timezone. Intended to be used to interpret
// time-based dimensions like `hour` and `minute`. Formatted as strings
// from the IANA Time Zone database ...
optional string time_zone = 6;For a property on America/Los_Angeles every OccurredAt and LastOccurredAt is 7-8 hours off, with no error and no way for a consumer to notice. It also falsifies the PR's own claim that OccurredAt "carries the UTC hour-bucket start", and the companion module repeats that claim in a GraphQL field description.
RunReportResponse.Metadata.TimeZone arrives with every response, so no extra call is needed — parse the bucket as local to that zone and convert explicitly. Consider DateTimeOffset on the public model so the semantics cannot be lost again.
Same root cause, smaller: DefaultEndDate = "today" resolves in the property's calendar while callers supply UTC instants.
There was a problem hiding this comment.
Fixed in 217d57e.
ParseDateHour now takes the zone from RunReportResponse.Metadata.TimeZone and converts to UTC explicitly (TimeZoneInfo.TryFindSystemTimeZoneById, so the IANA id resolves on every platform since ICU). An absent or unresolvable id leaves the bucket as it was read rather than failing the report, and IsInvalidTime guards the hour a spring-forward skips, which would otherwise throw. Tests: GetRowsAsync_PropertyTimeZone_ConvertsTheHourBucketToUtc plus a theory over null / empty / unknown ids.
What I did not change is the request direction, and it is worth recording why: From/To are still formatted as UTC dates, so a range boundary can still be off by up to a day in the property's calendar, and DefaultEndDate = "today" still resolves there. Closing that needs the zone before the first request, and it only arrives with a response — so it would cost a metadata round trip per read to fix a boundary nobody has reported. Going into the PR description as a documented limit instead.
I left the model as DateTime rather than moving to DateTimeOffset: the value is now genuinely UTC, and the type change would ripple through the consumer's GraphQL schema for no additional information.
|
|
||
| return await _platformMemoryCache.GetOrCreateExclusiveAsync(cacheKey, async cacheOptions => | ||
| { | ||
| cacheOptions.AbsoluteExpirationRelativeToNow = GetCacheTtl(settings); |
There was a problem hiding this comment.
major — this line does two unintended things, both silent.
GetOrCreateExclusiveAsync hands the factory the platform's default entry options and then uses that same object for the Set:
// vc-platform MemoryCacheExtensions.cs
var options = cache is IPlatformMemoryCache platformMemoryCache
? platformMemoryCache.GetDefaultCacheEntryOptions() : new MemoryCacheEntryOptions();
result = await factory(options);
if (!CacheDisabler.CacheDisabled && (result != null || cacheNullValue)) { cache.Set(key, result, options); }// vc-platform PlatformMemoryCache.GetDefaultCacheEntryOptions()
if (!CacheEnabled) { result.AbsoluteExpirationRelativeToNow = TimeSpan.FromTicks(1); }
else if (AbsoluteExpiration != null) { result.AbsoluteExpirationRelativeToNow = AbsoluteExpiration; }
else if (SlidingExpiration != null) { result.SlidingExpiration = SlidingExpiration; }1. The sliding expiration is left in place. With the shipped default (CacheSlidingExpiration: "0:15:00", no absolute configured) both expirations are honoured and whichever fires first evicts — so the documented CacheTtlMinutes (default 60) behaves as a 15-minute idle timeout. A dashboard opened every 20 minutes never hits, whatever the setting says.
2. Caching:CacheEnabled=false becomes inert for this cache. The platform implements that switch purely as a one-tick TTL, and this line overwrites it with at least a minute (GetCacheTtl floors at Math.Max(1, ...)). An operator flipping the platform switch to diagnose stale reporting data sees no change.
Clearing SlidingExpiration and honouring CacheEnabled are both one line each; the second could reasonably be a deliberate opt-out given the quota argument, but then it should say so at the site.
There was a problem hiding this comment.
Fixed in 217d57e — and thank you, this was the highest value-per-line finding in the review.
ApplyCacheTtl now: returns early when the platform has already stamped the one-tick disabled marker (so Caching:CacheEnabled=false works again), clears SlidingExpiration, and treats ttl <= 0 as "off" — the Math.Max(1, ...) floor is gone, so CacheTtlMinutes: 0 becomes a usable operator switch instead of silently meaning one minute.
It is deliberately the same shape as StatisticsCache.Apply in vc-module-sales-rep, which already solved exactly this for the statistics family. So this was a consistency gap with a pattern we already had, not a novel fix — worth knowing if you review that module.
One piece of context I'd add to the finding: "set absolute, leave sliding" is what every caller of GetOrCreateExclusiveAsync in the workspace does — of the four files that set an absolute TTL at all, none clears the sliding one, StatisticsCache included. The 15-minute idle window is a property of the platform helper rather than something this module got uniquely wrong. Diverging here is still right, because this is the only one of the four that publishes its TTL as an operator-facing setting with a documented number — but it is a divergence, and the PR description will say so.
One knock-on: AnalyticsDataApiSettings.CacheTtlMinutes now defaults to the descriptor's 60. Without that, any settings object built without an explicit TTL read as 0, which under the new rule means "disabled" — three existing tests caught it immediately. New tests: SearchEventsAsync_PlatformCachingDisabled_ReadsEveryTime, SearchEventsAsync_CacheTtlZero_ReadsEveryTime.
| // Count-mode rows carry no date, so the summaries come back with a null LastOccurredAt that the probe fills. | ||
| var summaries = CreateSummaries(criteria, totals.Events); | ||
|
|
||
| await Parallel.ForEachAsync( |
There was a problem hiding this comment.
major — one failed probe discards the successful totals read.
CreateSummariesAsync reads the totals, builds real summaries, then fans the per-name date probes out here. A single throwing probe faults the whole Parallel.ForEachAsync, which propagates out of the factory into the handler above:
LogFailure(operation, criteria.StoreId, ex);
cacheOptions.AbsoluteExpirationRelativeToNow = FailureCacheTtl; // 60 s
return createEmptyResult(); // every TotalCount = 0So a transient failure on one of up to 50 probes turns real counts into zeros for every requested name, and caches that for the failure TTL. A consumer rendering counts shows "0 events" rather than "unavailable".
Preserving the totals and representing a failed last-occurrence lookup separately from a genuine null would keep the partial result useful. GetEventSummariesAsync_SourceFails_... makes every call throw, so the partial case is currently unexercised.
There was a problem hiding this comment.
Fixed in 217d57e — each probe now carries its own try/catch, so a failure costs that event name's LastOccurredAt and nothing else. The totals stand.
Test: GetEventSummariesAsync_OneProbeFails_KeepsTheTotalsAndOnlyLosesThatLastOccurrence — one probe throws, and the assertion is that the other summary keeps both its count and its date while the failing one keeps its count with a null date.
Writing it turned up a second small thing worth mentioning: the existing helper stamped dates onto the totals rows, which count mode never returns. The new test uses a CreateCountModeResult fixture instead, so the probe is actually the only thing that can supply a date — otherwise the assertion would have passed for the wrong reason.
| $"{ApplicationDefaultCredentials} are not available — point GOOGLE_APPLICATION_CREDENTIALS at a service account key file, " + | ||
| "make sure the workload identity federation or metadata-server identity is reachable, " + | ||
| "or run 'gcloud auth application-default login' for local development.", | ||
| ex.Message); |
There was a problem hiding this comment.
major — the credential file path reaches the response payload.
ex.Message is forwarded verbatim as the check's Detail. Read at the pinned version (Google.Apis.Auth 1.73.0, DefaultCredentialProvider.cs), that message is:
throw new InvalidOperationException(
$"Error reading credential file from location {credentialPath}: {e.Message}..." +
$"Please check the value of the Environment Variable {CredentialEnvironmentVariable}.", e);So a present-but-malformed key file puts its absolute path into the HTTP response. Two more channels for the same class: MapApiAccessFailure interpolates the Cloud project id out of ErrorInfo.Metadata["consumer"] (pinned by ApiAccess_ServiceDisabled_MapsToEnableApiMessageWithProject), and DescribeError forwards raw RpcException.Status.Detail — provider-authored, unbounded — from four sites.
Key material itself does not leak and the endpoint is permission-gated, so the fix is small: either sanitise Detail to a mapped reason, or narrow the docs' claim that diagnostics "never reports a credential's contents — only its kind". Right now the disclosure is undeclared, and the declaration is the half a consumer upgrades on.
There was a problem hiding this comment.
Docs narrowed in 217d57e, and I'd push back on the severity — I think this is a minor.
The endpoint is gated on googleanalytics:access, and an absolute file path (or the Cloud project id from ErrorInfo.Metadata["consumer"]) is environment detail rather than credential material — the operator reading it is the same person who configured it, and the path is what they need in order to fix the thing the stage is telling them is broken. Sanitising detail to a mapped reason would cost the stage most of its diagnostic value.
What was wrong is the half you put last: the claim was broader than the code. docs/index.md now says key material never reaches the response, that the credentials stage reports only the kind, and — explicitly — that detail forwards Google's own message verbatim and that those messages can name the path in GOOGLE_APPLICATION_CREDENTIALS or the project behind a disabled API, which is why the endpoint is permission-gated.
If you'd still rather have the payload narrowed than the claim, say so and I'll map detail for the credentials stage specifically — it's the one stage where the message is Google's rather than ours.
|
|
||
| public interface IAnalyticsService | ||
| { | ||
| Task<bool> IsConfiguredAsync(string storeId); |
There was a problem hiding this comment.
major (design, see the summary) — the API cannot say "broken", and it cannot be cancelled.
Two decisions worth taking now rather than after release, because both are breaking changes to revisit:
- No member can report a failure. Every failure mode collapses into the same empty result, so a consumer showing "no activity" cannot tell a quiet customer from a misconfigured property. A status or envelope on the result closes it.
- No
CancellationTokenon any member here, onIAnalyticsDataSource, or on the fiveIGoogleAnalyticsReportClientmembers — while the generated Google client accepts one and the summary fan-out discards the loop token (async (summary, _) =>). Adding a parameter later changes the arity compiled callers reference and every implementer's signature.
One inconsistency inside the current shape: on the failure path CreateSummaries returns an empty list when EventNames is empty, because the projection falls back to the aggregate keys. The comment "Not an empty list: requested event names still yield zero-count summaries" holds only when names are supplied.
There was a problem hiding this comment.
Not taking either in this PR. The CancellationToken half is the one worth arguing, so here is the measurement behind it:
takes CancellationToken |
|
|---|---|
ICrudService / ISearchService (the two platform interfaces nearly every VC service derives from) |
no |
module I*Service.cs in vc-sources |
5 of 204 |
And the consumer that would have to thread it — ISalesRepActivitySource, also brand new — doesn't take one either, so a token here would dead-end one call away. Adding it buys a cancellation nothing propagates, at the price of being the exception to the platform's own contracts. If those move, this should move with them; I'd rather it be consistent than locally ideal.
On the error channel: I agree it's a real gap and I agree it's cheaper now than after release. I'm still leaving it, because the shape of the answer is a design decision wider than this PR — the diagnostics endpoint exists precisely so an operator can ask "is it broken", and the first consumer's product decision is that reps are deliberately never shown configuration state. An envelope added late, to satisfy a consumer that has decided not to surface it, is the kind of API that ends up with one caller ignoring it. Worth its own ticket.
The sub-point at the end of your comment was right and is fixed: CreateSummaries does return an empty list when EventNames is empty, so the comment above CreateEmptySummaries now says the fallback holds only when names are supplied.
| serviceCollection.AddTransient<IAnalyticsService, AnalyticsService>(); | ||
| serviceCollection.AddTransient<IAnalyticsDiagnosticsService, AnalyticsDiagnosticsService>(); | ||
|
|
||
| AbstractTypeFactory<AnalyticsDataApiSettings>.RegisterType<AnalyticsDataApiSettings>(); |
There was a problem hiding this comment.
minor — these twelve registrations are redundant, and they have a side effect.
All twelve types are concrete, have parameterless constructors and carry no discriminator, and every construction site uses the parameterless TryCreateInstance() — the case where the factory's fallback already compiles new X() without any registration.
The part worth knowing is what the registration switches on:
// AbstractTypeFactory<T>
public static bool HasOverrides => Volatile.Read(ref _typeInfosSnapshot) is { Length: > 0 };
// PolymorphJsonConverter.CanConvert
return _canConvertCache.GetOrAdd(objectType, _ =>
(bool)typeof(AbstractTypeFactory<>).MakeGenericType(objectType)
.GetProperty("HasOverrides").GetValue(null, null));Registering a base as itself makes HasOverrides true, so the converter now claims these types and [FromBody] AnalyticsDiagnosticsRequest round-trips through JObject.Load + Populate instead of the normal binder. No behaviour difference is observable today — the list initialisers are empty and the bool default survives an absent field — but it is a deserialization path nobody asked for, acquired from a line that does nothing else.
There was a problem hiding this comment.
Leaving these — but the mechanism you traced is correct and worth knowing, so thank you for digging it out. I verified both halves: HasOverrides flips true on any registration including a self-registration, and PolymorphJsonConverter.CanConvert reads exactly that property, so these types really are routed through JObject.Load + Populate rather than the normal binder.
Before deciding I counted self-registrations (AbstractTypeFactory<X>.RegisterType<X>() where the argument is the registered type) across all of vc-sources: 40 across 9 repos, with vc-platform itself accounting for 11. So it is a sparse-but-real practice rather than either a settled convention or a lone outlier — which leaves the decision resting on nothing but taste, given (as you say) no observable behaviour difference today.
Twelve lines of churn in either direction is a coin flip, so I'm spending the diff elsewhere. If you'd like them gone, say so and they go in the next round — I have no argument for keeping them beyond inertia.
| : string.Empty; | ||
| var eventCounts = AggregateEventCounts(response.DimensionHeaders, response.Rows); | ||
|
|
||
| if (eventCounts.Count == 0) |
There was a problem hiding this comment.
minor — Passed while the events the caller asked about are absent.
DescribeMissingRequestedEvents appends "Requested events not seen: ..." to a Passed row. So an operator who asked "is view_item being collected?" gets a green status plus a sentence — and a UI colours by status, not by prose. Warning is the value that matches the message; the happy-path test currently locks the green in while asserting the requested event is missing.
Two smaller ones nearby:
- The realtime fallback (
catch (RpcException ...) when InvalidArgument) retries without the custom dimensions and never records the original exception — the one naming which dimension GA4 rejected. If the retry also fails, only the second failure is reported. CollectFilterFieldNameshandles onlyFilterandAndGroup, and the collected set is used to filter the compatibility findings — so a field reached through anOrGrouporNotExpressionis dropped and the stage reportsPassed. The in-repo builder only emitsAndGroup, but both methods areprotected virtual.
There was a problem hiding this comment.
Fixed in 217d57e — the status now carries the answer.
Both data stages take their status from the missing set (GetDataStatus: Warning when a requested event was not seen, Passed otherwise). You were right that the happy-path test was locking the wrong behaviour in — its fixture now returns both requested events, so "happy" means what it says, and two new tests own the missing case: Realtime_RequestedEventNotSeen_WarnsAndNamesIt and ProcessedData_RequestedEventNotSeen_WarnsAndNamesIt.
Realtime fallback: fixed too. The original RpcException is kept across the retry — reported as the check's detail when the retry succeeds, and appended to the failure when the retry fails as well, so the message naming which dimension GA4 rejected is never the one that gets dropped.
CollectFilterFieldNames (OrGroup / NotExpression): noted, not fixed. The in-repo builder emits only AndGroup, and a subclass that emits others would be overriding the request builder — the same seam it would override to collect them. I'd rather not add traversal for a shape nothing produces.
| <TargetFramework>net10.0</TargetFramework> | ||
| <OutputType>Library</OutputType> | ||
| <!--This line is necessary to copy all dependencies in the bin folder--> | ||
| <CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies> |
There was a problem hiding this comment.
minor — the flag is required, but module.ignore was not updated with it.
Without CopyLocalLockFileAssemblies the Google assemblies never reach the probing folder, so this has to be here. It also copies the whole closure: from this branch's project.assets.json the .Web output now includes Newtonsoft.Json, Serilog, System.Management, System.CodeDom, System.IO.Abstractions, TimeZoneConverter, VirtoCommerce.Platform.Core and VirtoCommerce.StoreModule.Core — all of which the platform already provides.
module.ignore lists only TimeZoneConverter.dll; sibling modules that set this flag list the platform-provided set instead. Nothing breaks today (the one genuinely shared library resolves to the same Newtonsoft.Json 13.0.4 on both sides, and the platform references no Grpc/Google/Protobuf package, so no version ceiling is exceeded) — the cost is artifact bloat now and a silent FileNotFoundException at module load the day any of those resolves above the host's copy.
There was a problem hiding this comment.
Fixed in 217d57e.
Rather than copy a sibling module's list I diffed the .Web output against the running platform: 15 of the 31 copied assemblies are already provided by the host or by a dependency module, and those are now listed — VirtoCommerce.Platform.Core.dll, Newtonsoft.Json.dll, Serilog.dll, System.CodeDom.dll, System.Management.dll, the System.IO.Abstractions trio, TimeZoneConverter.dll, and the four VC module .Core assemblies that arrive through StoreModule.Core.
What still ships is exactly the Google/Grpc chain plus this module's own three assemblies, which is the set that has to be there for the flag to be doing its job.
| } | ||
|
|
||
| [Fact] | ||
| public void Constructor_BypassesAnalyticsCacheAndService() |
There was a problem hiding this comment.
nit — this test cannot fail for the property its name claims.
The name asserts a behavioural relation ("bypasses the analytics cache"); the body asserts only that three types are absent from the constructor's parameter list. The defect it is named for is reachable without touching them — routing diagnostics through IAnalyticsDataSource (cached one layer up), or caching inside GoogleAnalyticsReportClient, which is already a constructor parameter — and both leave it green.
The property is observable through the existing _reportClientMock: assert the client is called twice across two consecutive RunAsync calls with the same arguments.
There was a problem hiding this comment.
Fixed in 217d57e — replaced with RunAsync_CalledTwiceWithTheSameArguments_ReadsGoogleEachTime, which asserts the client is hit twice across two identical runs (metadata, realtime, and the processed-data report).
Your counterexample is what decided it: caching inside GoogleAnalyticsReportClient — already a constructor parameter — would have left the old assertion green while the stage served a stale verdict, which is the failure that actually matters.
| } | ||
|
|
||
| [Fact] | ||
| public async Task SearchEventsAsync_SameCriteria_UsesCache() |
There was a problem hiding this comment.
nit — the cache-hit test uses the one input shape that always hits.
CreateSearchCriteria() sets To to a compile-time constant, so this test cannot detect the cache-key precision issue: the key is built by ValueObject.GetCacheKey() over every public property, which renders From/To at second precision, while the GA request formats them as yyyy-MM-dd. Two calls differing only in sub-day time therefore produce the same Google query under different keys.
The mutation that shows it: build the criteria with DateTime.UtcNow in both calls and the Times.Once assertion fails with the production code unchanged.
Worth deciding rather than just testing. The companion consumer's storefront already rounds its upper bound to end-of-day specifically because a raw instant defeated a backend cache — so the guarantee currently lives as a convention in a different repository, which every future consumer has to rediscover. Normalising From/To to the dates GA actually receives, inside this module, is the durable fix. One dependent to fix in the same change: the companion's featureQuery diagnostics stage currently bypasses the cache by accident (it passes DateTime.UtcNow.AddDays(-30)), and reading live is that stage's whole purpose — so it needs an explicit bypass once the key is normalised.
There was a problem hiding this comment.
Fixed in 217d57e, and normalised inside the module as you suggested rather than left as a convention every consumer has to rediscover.
SearchEventsAsync and GetEventSummariesAsync round From/To to dates before the cache key is built, on a clone so the caller's criteria is not mutated. Tests: SearchEventsAsync_CriteriaDifferingOnlyInTimeOfDay_UsesCache and SearchEventsAsync_NormalizingDates_DoesNotMutateTheCallersCriteria.
You were also right about the dependent, and it is handled in the same wave rather than left to be discovered: AnalyticsEventCriteriaBase gains a BypassCache flag (new public member — GetOrCreateAsync skips the cache entirely for it, failures included), and the sales-rep featureQuery diagnostics stage now sets it explicitly instead of depending on DateTime.UtcNow defeating the key. Test: SearchEventsAsync_BypassCache_ReadsEveryTime.
That does mean the sales-rep side needs the alpha this PR publishes before its own PR can build — the two are sequenced that way already.
|
Thanks for both reviews — they were unusually careful, and almost all of it survived being re-derived from source. Everything is in 217d57e; per-finding replies are inline. Fixed: the Declined, with reasoning inline: the Two cross-cutting points from the summary: The provider seam living in "Four new store-level settings." Correct, there are two. That and the artifact URL are going into the description with the next push. One thing this round added to the public surface, so it does not arrive unannounced: |
alexeyshibanov
left a comment
alexeyshibanov
left a comment
There was a problem hiding this comment.
Re-review — all 13 threads from the previous pass re-checked against 2c217b2
Verified by reading the current code, and where a fix claimed test coverage, by reverting that fix in a scratch copy and re-running the suite. Result: 8 fixed, 1 partially fixed, 4 declined with the stated reason holding. No decline rests on a false premise — I checked each mechanism rather than the argument: the permission gate and the absence of key material on the credentials stage; BuildItemReportRequest being reachable only through HasItemDimensions; ICrudService / ISearchService genuinely taking no CancellationToken; and AbstractTypeFactory.HasOverrides feeding PolymorphJsonConverter.CanConvert as described, with CreateFallbackInstance making the twelve self-registrations redundant but harmless.
Build clean, 78/78 green. module.ignore checks out line by line: 31 assemblies in the .Web output, all 15 ignore entries present in it, no stale lines, and the module never resolves a shared dependency higher than the host provides.
Two observations are cross-cutting rather than line-local.
1. A caller still cannot tell "no data" from "broken". This was declined on IAnalyticsService and the decline is defensible on its own terms, but three mechanisms now converge on it: a malformed dimension filter throws and is caught as an ordinary Google failure; a failed last-occurrence probe leaves a null with no marker; and the processedData stage reads through the same swallowing path, so a provider error renders as the "no processed rows yet" warning rather than a failure. Each is defensible alone. Together they make an empty result the module's single answer to every question, and an empty analytics result is one a consumer will act on. Worth settling before the surface ships, because adding a status afterwards is the breaking half.
2. Two fixes are in the code but pinned by nothing. Deleting options.SlidingExpiration = null, or the whole malformed-value catch in AnalyticsSettingsResolver, each leaves 78/78 green. Both are behaviours the docs promise, so both are silent-regression candidates for the next refactor.
Merge order. This PR is the head of a three-repo wave. The companions' green CI was validated against the prerelease build, so the run that actually matters is the one after they re-pin to the released 4.1004.0.
| return await ReadAsync(operation, criteria, factory, createEmptyResult, settings, null); | ||
| } | ||
|
|
||
| var cacheKey = CacheKey.With(GetType(), operation, criteria.GetCacheKey()); |
There was a problem hiding this comment.
The key covers the criteria completely — ValueObject.GetCacheKey() joins every public property, so event names, dimension filters, take/skip and sort are all in there — but it does not cover the settings the read was resolved against. Changing a store's GoogleAnalytics4.DataApi.PropertyId therefore keeps serving the previous property's numbers until the entry expires, up to CacheTtlMinutes (60 by default).
That is not a delayed refresh: it is a different property's data returned under the new configuration, with nothing in the response saying so, and the first thing an operator does after fixing a wrong property id is re-read the report. Folding the resolved settings.PropertyId into the key costs one argument.
There was a problem hiding this comment.
Fixed in 2712d8b — the key is now CacheKey.With(GetType(), operation, settings.PropertyId, criteria.GetCacheKey()). It also falls out of the wider rule: if only the Google call is cached, the key has to name what the rows are of. Pinned by SearchEventsAsync_PropertyIdChanged_DoesNotServeThePreviousPropertysRows.
There was a problem hiding this comment.
Verified at da8a37a: the key now carries the resolved settings.PropertyId, and SearchEventsAsync_PropertyIdChanged_DoesNotServeThePreviousPropertysRows pins it. Closing this one from my side.
| { | ||
| try | ||
| { | ||
| summary.LastOccurredAt = await GetLastOccurredAtAsync(settings, criteria, summary.EventName); |
There was a problem hiding this comment.
Isolating the probe failure was the right call, but it moved the cost rather than removing it. Because the exception no longer escapes the factory, ReadAsync's ApplyCacheTtl(cacheOptions, FailureCacheTtl) branch never runs — so a summary carrying a real TotalCount with a null LastOccurredAt is now pinned for the full configured TTL instead of 60 seconds.
The previous behaviour was wrong loudly (zeros, for a minute); this one is wrong quietly for an hour, and a "last login: never" reads as fact rather than as a gap. Shortening the entry's TTL when any probe failed keeps the isolation and drops the staleness.
There was a problem hiding this comment.
Fixed in 2712d8b, but by deleting the isolation rather than shortening the TTL — a failed probe now fails the whole summary read.
Your own sentence is the reason: "last login: never" reads as fact. And with reads throwing everywhere else, a partial result here would have been the one site in the diff answering "Google failed" differently. The entry is cached as a failure for 60 s, so the staleness you measured is gone either way.
There was a problem hiding this comment.
Resolved, though by the opposite move to the one I suggested: the isolation is gone and a failed probe now fails the whole summary read. Under the new throwing contract that is the better of the two — there is no longer a partial result a caller could read as fact, so the ambiguity is removed rather than time-boxed.
Worth recording that this reverses the fix asked for in the previous round's thread on these lines, which wanted the probe failure isolated so an already-successful totals read was not discarded. The harm named there was zeros presented as data, and that harm is gone now the failure reaches the caller. What replaces it is narrower: a transient failure on one probe costs the whole summary until the failure entry expires, where before it cost one null field. I think that is the right trade — flagging it only so the reversal sits on the record as a decision rather than a silent swing between rounds.
There was a problem hiding this comment.
Recorded in index.md
| return; | ||
| } | ||
|
|
||
| options.SlidingExpiration = null; |
There was a problem hiding this comment.
This line is the half of the cache fix that makes the documented CacheTtlMinutes mean what it says — without it the platform's 15-minute idle default evicts first, and the setting silently becomes an upper bound nobody reaches.
It is also the half nothing pins: SlidingExpiration appears once in src and zero times in tests, so deleting it keeps the suite green. Its two siblings from the same fix — the CacheEnabled=false sentinel and the removed Math.Max(1, …) floor — both have tests. Worth one assertion here so a future refactor cannot drop it unnoticed.
There was a problem hiding this comment.
Fixed in 2712d8b — SearchEventsAsync_ShorterSlidingDefault_DoesNotEvictBeforeTheConfiguredTtl: a 50 ms sliding default against a 5-minute TTL, re-read after 300 ms. Behavioural rather than an assertion on the options object; red-checked by deleting the line.
| // disable reporting rather than restore the global value. | ||
| var hasStoreValue = storeSettings?.Any(x => | ||
| x.Name.EqualsIgnoreCase(descriptor.Name) && | ||
| x.Value is not null and not "") == true; |
There was a problem hiding this comment.
not "" closes the cleared-value case but not the whitespace one. A store value of " " still passes this gate, still wins over the global value, and AnalyticsDataApiSettings.IsConfigured (!string.IsNullOrEmpty) still reports the store as configured — so reporting is off while the diagnostics configuration stage says a property id is set, and every call builds properties/ .
Same mechanism as the original finding, one instance further along. IsNullOrWhiteSpace on both this gate and IsConfigured covers it.
There was a problem hiding this comment.
Fixed in 2712d8b, both gates. IsStoreOverride treats whitespace as a cleared text setting and keeps the plain null check for the rest — 0 is a legitimate CacheTtlMinutes — and IsConfigured is now !IsNullOrWhiteSpace.
| { | ||
| return storeSettings.GetValue<T>(descriptor); | ||
| } | ||
| catch (Exception ex) when (ex is FormatException or InvalidCastException or OverflowException) |
There was a problem hiding this comment.
The catch list is right — Convert.ChangeType under SettingsExtension.GetValueInternal<T> fails in exactly these three ways — but nothing exercises it: deleting the whole block leaves the suite green. ResolveAsync_ClearedStoreValue_FallsBackToGlobal covers the other half of this method.
One test with a non-numeric store value for CacheTtlMinutes would pin it, since this is the path that keeps a misconfigured store readable instead of failing its reads.
There was a problem hiding this comment.
Fixed in 2712d8b — ResolveAsync_MalformedStoreValue_FallsBackToGlobal, a non-numeric store CacheTtlMinutes. Red-checked by disabling the when filter.
| // then means items viewed, not occurrences, and EventName is filled only when exactly one name was asked for. | ||
| Task<AnalyticsEventSearchResult> SearchEventsAsync(AnalyticsEventSearchCriteria criteria); | ||
|
|
||
| Task<IList<AnalyticsEventSummary>> GetEventSummariesAsync(AnalyticsEventSummaryCriteria criteria); |
There was a problem hiding this comment.
The item-scope caveat sits on SearchEventsAsync, where the consequence is a visibly absent EventName. On this method the same shape is worse, and undocumented: with an item-scoped dimension and two or more event names, BuildItemReportRequest omits the eventName dimension, the single-name back-fill in GetItemScopedRowsAsync does not apply, every row arrives with a null EventName, and CreateSummaries drops all of them. The caller gets zero-count summaries that are indistinguishable from "no activity".
No consumer in this wave reaches it — every item-scoped read names exactly one event — but this is a published API and the failure is silent numbers rather than a missing field. Either carry the caveat here too, or reject the combination outright.
There was a problem hiding this comment.
Rejected outright in 2712d8b: GetItemScopedRowsAsync throws on two or more event names, before the request is built. Checked the wave first — every item-scoped read names exactly view_item — so nothing downstream moves.
There was a problem hiding this comment.
Agreed on the outcome — refusing the combination is better than the documentation I offered as the alternative, and keeping a backstop in GetItemScopedRowsAsync covers a consumer resolving IAnalyticsDataSource directly, which the service-entry check alone would not.
One correction for the record, since this thread will be read later than it was written: the guard is new in 2712d8b. At 2c217b2, the commit this review was written against, GetItemScopedRowsAsync carried no event-name check — it back-filled EventName when exactly one name was requested and left it null otherwise, which is the path the comment described. So this is a fix rather than a rejection; as the thread stands it reads as though the guard predated the review.
There was a problem hiding this comment.
Correct, and the correction is fair — the guard is new in 2712d8b, not pre-existing. "Rejected outright" described the shape of the decision, not its date, and it reads as the latter. Recording it here: this was a fix made in response to the finding.
| expressions.Add(AnalyticsFilterBuilder.CreateInListExpression(ModuleConstants.Dimensions.EventName, query.EventNames)); | ||
| } | ||
|
|
||
| foreach (var filter in (query.DimensionFilters ?? []).Where(x => !string.IsNullOrEmpty(x.DimensionName))) |
There was a problem hiding this comment.
A filter carrying no values now throws rather than widening the read. A filter carrying an empty DimensionName is still dropped silently by this predicate, which widens it exactly the same way — same failure mode, different field.
There was a problem hiding this comment.
Fixed in 2712d8b — an empty or whitespace DimensionName throws in the same block as the empty Values, and the loop no longer filters them out on the way in.
| if (filter.Values.IsNullOrEmpty()) | ||
| { | ||
| throw new ArgumentException( | ||
| $"Dimension filter '{filter.DimensionName}' carries no values, which would leave the read unscoped.", |
There was a problem hiding this comment.
The guard does the job it exists for: no unscoped read reaches Google. What it cannot do is say so. AnalyticsService.ReadAsync catches this with everything else, logs a warning, stamps the 60-second failure TTL and returns an empty result — so a consumer that passed a malformed filter is told "no data", and only a consumer resolving IAnalyticsDataSource directly ever sees the exception.
Not an argument against the guard, which is right where it is. It is worth knowing when reading the comment above it, which reads as though the refusal is observable to whoever caused it.
There was a problem hiding this comment.
Resolved by the design change: ReadAsync no longer swallows, so the refusal reaches whoever caused it as an ArgumentException instead of as "no data". The comment above the guard is rewritten to cover both cases.
| public DateTime? To { get; set; } | ||
|
|
||
| // For a diagnostics probe: a feature read must not set it, since the cache is what protects the quota. | ||
| public bool BypassCache { get; set; } |
There was a problem hiding this comment.
Nit. This is a public settable flag that switches off what the module's own docs call a quota requirement rather than tuning, and the only thing keeping a feature read from setting it is this comment. Nothing in the repo sets it today.
The shape to watch is a consumer binding an AnalyticsEventSearchCriteria from caller-supplied input — the flag then rides along with it. Narrower access, or a separate criteria type for the probe, would make the rule structural rather than advisory.
There was a problem hiding this comment.
Deferred, not disagreed — a parameter would make the rule structural instead of a comment. Leaving it while this surface is still moving; noted for the pass that changes these signatures.
| tuning; failed reports are cached for a fixed 60 seconds so a misconfiguration cannot burn quota. Set it to | ||
| **`0`** to read Google on every call, and note that the platform's own `Caching:CacheEnabled=false` is honoured | ||
| here too - both are for diagnosing stale reporting data, not for normal operation. The cache key is the store | ||
| plus the query's *dates*, so two reads differing only in time of day share one entry. |
There was a problem hiding this comment.
Nit. The key is CacheKey.With(GetType(), operation, criteria.GetCacheKey()), and ValueObject.GetCacheKey() joins every public property — event names, dimension filters, dimension names, take/skip, sort. "The store plus the query's dates" describes the date normalisation, not the key.
It matters because this is the sentence a reader reasons from when deciding whether two reads share an entry, and read literally it says two different event-name queries collide.
There was a problem hiding this comment.
Fixed in 2712d8b — the sentence now says the key covers the whole query plus the resolved property id, with the day-rounding named separately as the reason two times of day share one entry.
alexeyshibanov
left a comment
alexeyshibanov
left a comment
There was a problem hiding this comment.
Approving. Every thread from both of my passes is answered, and the three changes since da8a37a each close one of them at the source rather than around it.
What I verified at bd937ca:
- The seam claim is now honest.
IAnalyticsDataSource's comment states it is deliberately not a provider boundary, and names why —AnalyticsDataQueryis GA4-shaped. Retracting the claim is the better answer than engineering toward it, and it is the one that stops a downstream reader building against a substitutability that was never there. - The probe reversal is documented where a consumer reads it, including its cost: one transient probe failure now costs the whole summary until the failure entry expires. That trade being written down is what makes it a decision rather than a swing between rounds.
AnalyticsFilterBuilderhas its own tests (+202), reached throughInternalsVisibleTo. That was the last thing carrying the dimension-filter guard without a direct test.
One item left, and it is a one-line edit rather than a reason to hold the PR:
The description's opening paragraph still says "a generic, source-agnostic analytics read API", which is the claim the new IAnalyticsDataSource comment withdraws. The service-table row was updated in the same round; this sentence was not, so the description now contradicts itself, and this is the first line anyone reads. Both the code comment and docs/index.md are correct — it is only the PR text that is behind.
Worth a note for whoever picks it up next: the BypassCache thread was deferred on the grounds that "this surface is still moving". That reasoning expires at merge, so if the flag is to be narrowed, this is the last cheap moment.
Merge order unchanged — this PR releases 4.1004.0 first, then the companions re-pin off the prerelease and their CI re-runs against the released surface.
# Conflicts: # Directory.Build.props # src/VirtoCommerce.GoogleEcommerceAnalyticsModule.Data/VirtoCommerce.GoogleEcommerceAnalyticsModule.Data.csproj # src/VirtoCommerce.GoogleEcommerceAnalyticsModule.Web/module.manifest # tests/VirtoCommerce.GoogleEcommerceAnalyticsModule.Tests/VirtoCommerce.GoogleEcommerceAnalyticsModule.Tests.csproj
GoogleAnalytics4.DataApi.RequestTimeoutSeconds, global and per store like the cache lifetime, becomes the deadline of every call the module makes to Google, diagnostics included. The client library's own default (60 s, never retried) matched the ~60 s an ingress gives a whole request, so a slow Google answer surfaced as a gateway timeout of the consumer's request instead of a failed read the consumer degrades on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every report now asks Google for the metric total (metricAggregations: TOTAL), mapped to AnalyticsEventSearchResult.TotalEventCount: the metric summed over every matching row, whatever page was requested. TotalCount stays the row count a pager needs; a consumer that wants to say how many searches or views happened no longer has to count hour rows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A summary made a totals read first and only then the newest-bucket probes, so its worst case was two request timeouts back to back. Each name is now one read that returns the newest hour bucket with Google's total over every bucket beside it; the reads of a named summary need nothing from each other and start together. Only a summary that names no events still reads which events exist first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A cleanup pass with no change in behaviour. The settings failure path, the item-scope refusal and metric parsing each live in one place now. A store setting value converts through the platform's SettingValueConverter.TryConvert, with one fallback to the global value. GA4 resource names come from Google's PropertyName and MetadataName, and diagnostics prefix user dimensions through their own MapDimensionName seam, as their requests' dimensions already did. Diagnostics messages take the setting name, the scope and the time windows from the values that define them. A summary's last hour is its one row's. The service tests mock IAnalyticsDataSource and share one row setup, and the controller test uses Moq instead of two fakes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit de1a54d. Configure here.
| await Parallel.ForEachAsync( | ||
| Enumerable.Range(0, eventNames.Count), | ||
| new ParallelOptions { MaxDegreeOfParallelism = MaxConcurrentReads }, | ||
| async (index, _) => summaries[index] = await GetSummaryAsync(settings, criteria, eventNames[index])); |
There was a problem hiding this comment.
Summary reads exceed request timeout
Medium Severity
CreateSummariesAsync only runs four Google reads at a time, so a summary of the seven built-in events runs in two waves. Each wave can last the full RequestTimeoutSeconds (45), which can outlast the ~60 second ingress limit that timeout is meant to stay under. That failure is then cached.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit de1a54d. Configure here.
| request.Dimensions.Add(new Dimension { Name = ModuleConstants.Dimensions.EventName }); | ||
| request.Metrics.Add(new Metric { Name = ModuleConstants.Metrics.EventCount }); | ||
|
|
||
| return request; |
There was a problem hiding this comment.
Diagnostics truncates live event counts
Medium Severity
Realtime and processed-data checks request at most 50 rows, with no sort and no filter for the requested event names. GetMissingRequestedEvents treats any name missing from that page as not collected. Extra realtime dimensions, or more than 50 event names, can warn that events are missing and under-count them even when Google has them.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit de1a54d. Configure here.
| .Select(x => x.EventName) | ||
| .Where(x => !string.IsNullOrEmpty(x)) | ||
| .DistinctIgnoreCase() | ||
| .ToList(); |
There was a problem hiding this comment.
Item-scoped summaries drop event names
Medium Severity
When a summary names no events, GetEventNamesAsync reads names from each row's EventName. An item-scoped filter sends that query through GetItemScopedRowsAsync, which leaves EventName empty unless the query already has one event name. Every discovered name is dropped, and the summary returns empty instead of those events.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit de1a54d. Configure here.
0 turns the cache off, as the docs say, but the setting was a PositiveInteger, which the platform refuses to save as 0. It is an Integer now; the read already treats 0 and below as no cache. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An organization name can be a person's name (a sole trader's), and GA forbids personal data in custom dimensions. The storefront stops sending it (VirtoCommerce/vc-frontend#2458), and nothing reads it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
## Description The host side of **Customer insights: GA**: the tagging that makes per-customer GA4 reports possible, a way for a menu link to say which pages it owns, and the `@vc-frontend/core` exports the Sales Rep Hub plugin needs. The insight surfaces themselves — the activity feed, the My activity dashboard card and the Customer activity profile widget — moved into the hub's plugin (VirtoCommerce/vc-module-sales-rep#15) once the hub became a Module Federation plugin (#2569); nothing hub-specific is left here. Companion PRs: - VirtoCommerce/vc-module-google-ecommerce-analytics#61 — reads GA4 through the Data API (`IAnalyticsService`) - VirtoCommerce/vc-module-sales-rep#15 — the `salesRepActivities` / `salesRepCustomerActivitySummary` / `salesRepCustomerInsights` queries and the hub UI on top of them; its plugin builds against this PR's facade ## Behaviour - **Customer identity on every GA4 event** (`modules/google-analytics/user-properties.ts`): `contact_id`, `organization_id`, `session_kind` and `is_sales_rep` go out with `gtag('set', 'user_properties', …)` **ahead of GTM's `gtm.js` entry and the first `config`**, so the initial `page_view` is tagged too — the data layer is processed in order, and GA never backfills an event that went out untagged. Every property is sent every time. A signed-out visitor is tagged explicitly — `session_kind: "anonymous"`, `is_sales_rep: "false"`, the ids `null` — because GA keeps the most recent value of a user property per browser and neither an omitted key nor `undefined` clears it: without that, a visitor browsing after a sign-out was counted as the last customer. Re-applied when the user changes in place (another tab signs in), and pushed to `dataLayer` as well when a GTM container is configured, since GTM cannot read gtag's internal state. `is_sales_rep` reads raw permission membership, not `checkPermissions` — an administrator is granted everything and reporting them as a sales rep would be worse than reporting nothing. The organization's **name** is not sent: it is free text that can be a person's name (a sole trader), GA forbids personal data in custom dimensions, and reports join on `organization_id`. The four names must be registered by hand in GA4 Admin; the table is in [the module README](client-app/modules/google-analytics/README.md). - **A sign-in is reported from the page it lands on** (`modules/google-analytics/pending-login.ts`): signing in navigates away at once, so a `login` sent on the spot raced the page unload and carried the signed-out identity — most never arrived. A successful sign-in is parked in `sessionStorage` and sent once, after the next page has applied the new identity and GA's `config`; it is removed before it is sent, kept for two minutes at most and dropped if that page is not signed in, so it can never repeat. When no reload comes within 10 seconds — a step after the token failed and the visitor stayed on the form — the sign-in page sends it itself, under the identity it still has, so a login is never lost; a retry restarts that wait, so an earlier attempt's timer cannot take the retry's login. A failed sign-in is still sent at once, and the form (`sign-in-form.vue`) now reports one event per attempt: a sign-in that throws is a failure only (it used to fall through to a success as well), and a wrong password is reported once, with its identity errors. - **A menu link can say which pages it owns** (`activeWhen` on `ExtendedMenuLinkType`, `isMenuLinkActive` in `core/utilities/menu`): the account rail (`account-navigation-item.vue`) and the mobile menu (`mobile-menu-link.vue`) light a link by its own rule when it has one, and by vue-router's record match otherwise. The Sales Rep Hub uses it to light My customers on a customer's profile, orders and activity pages, which are sibling route records of the list rather than its children. Not fixed here: `VcMenuItem` sets `aria-current="page"` from vue-router's match and gives the caller no way to override it, so a link lit by its own rule is not announced as the current page; that needs a UI-kit change. - **`@vc-frontend/core` 0.2.4** (`client-app/core-api`): exports `isMenuLinkActive` (a plugin that draws its own account-menu link highlights it the way the rail does), `QueryParamName` (the query-string names host pages read, for a plugin that links to search results) and `VcTooltip`, and the contract carries `activeWhen` on the menu link type. Once this merges, the **Core Facade Release** workflow publishes `core-v0.2.4`. The hub plugin requires a host on 0.2.4 or newer — a storefront below that skips the whole plugin — so this PR's build has to reach an environment before the plugin's does. ## Setting up GA4 for customer insights The insight widgets do not read the storefront database — they read **GA4 through the Data API**. Two independent halves have to be configured: the store must *send* tagged events, and the platform must be allowed to *read them back*. Until both are done the widgets show an "analytics unavailable" state rather than an empty list, so a tester can tell a misconfiguration from a quiet customer. ### 1. Enable tracking on the store Back Office → open the store → **Store Settings** → **Google Analytics 4** section: | Setting | Value | | --- | --- | | `GoogleAnalytics4.EnableTracking` | on | | `GoogleAnalytics4.MeasurementId` | `G-XXXXXXXXXX` | <img width="1383" height="844" alt="image" src="https://github.com/user-attachments/assets/50062eb0-5f44-4ad7-a550-ac7e3fc75025" /> ### 2. Register the four custom definitions in GA4 Without this step the events are still collected, but nothing can be narrowed to one customer: GA4 has no built-in dimension for the signed-in user, and unregistered user properties are discarded for reporting. GA4 Admin → **Data display → Custom definitions → Custom dimensions → Create custom dimension**. Create four, each with **Scope = User**: | Dimension name | Scope | User property | Value it carries | | --- | --- | --- | --- | | `contact_id` | User | `contact_id` | Contact id — the join key back to the platform | | `organization_id` | User | `organization_id` | Organization id; this is what the widgets filter on | | `is_sales_rep` | User | `is_sales_rep` | `true` when the account holds `sales-rep:access`, otherwise `false` | | `session_kind` | User | `session_kind` | `self`, `impersonated` while an operator drives the session, or `anonymous` once signed out | A property set up for an earlier build of this PR can archive its `organization_name` dimension: nothing sends it any more. <img width="902" height="425" alt="image" src="https://github.com/user-attachments/assets/1341b933-6164-43a0-9b24-0dd1086d59c3" /> Four rules that bite: - **The user-property string must match exactly.** The names come from `client-app/modules/google-analytics/user-properties.ts`; a typo silently collects nothing. - **Scope must be User, not Event.** An event-scoped dimension of the same name stays empty here. - **GA does not backfill.** A dimension reports only on data collected *after* it was registered, so register all four before generating test traffic — and, on a client property, as early in onboarding as possible. - A standard GA4 property allows a limited number of user-scoped custom dimensions (25 at the time of writing); these four count against that budget. ### 3. Find the numeric property id GA4 Admin → **Property settings → Property details**. The **Property ID** is the numeric one, e.g. `123456789` — it is shown on that page and also appears in any GA4 URL as `p123456789`. It is **not** the `G-XXXXXXXXXX` measurement id: the measurement id identifies the data stream that *receives* events, the property id identifies what the Data API *reads*. <img width="1491" height="134" alt="image" src="https://github.com/user-attachments/assets/ab692576-2533-4666-a2cd-7648cc252f8a" /> ### 4. Enter the reporting settings Same Store Settings screen as step 1: | Setting | Value | | --- | --- | | `GoogleAnalytics4.DataApi.PropertyId` | the numeric id from step 3 | | `GoogleAnalytics4.DataApi.CacheTtlMinutes` | `60` by default; set `0` while testing so a change is visible on the next page load instead of an hour later | | `GoogleAnalytics4.DataApi.RequestTimeoutSeconds` | `45` by default: how long one Google read may take before the widgets say web activity isn't available instead of waiting on it | All three are non-public settings — they are never returned by the anonymous `GET /api/googleanalytics/{storeId}` endpoint. ### 5. Let the platform read the property (once per environment) 1. Enable the **Google Analytics Data API** (`analyticsdata.googleapis.com`) in the Google Cloud project the credential belongs to. 2. Provide **Application Default Credentials**. There is deliberately no credential setting, so no key material is stored in the platform database: in a cluster this is workload identity or the metadata server, with a key file it is `GOOGLE_APPLICATION_CREDENTIALS`. For local development the login must carry the analytics scope — a plain `gcloud auth application-default login` does not, and every call then fails with `PermissionDenied`: ```sh gcloud auth application-default login --scopes="https://www.googleapis.com/auth/analytics.readonly,https://www.googleapis.com/auth/cloud-platform,https://www.googleapis.com/auth/userinfo.email,openid" gcloud auth application-default set-quota-project <your-gcp-project> ``` 3. Grant that principal the **Viewer** role on the property: GA4 Admin → **Property access management**. ### 6. Verify the setup before testing the widgets This is a **Back Office** page, not a storefront one: open the **Sales Rep** application and pick **Analytics diagnostics** in its left menu (the item is visible only with the `sales-rep:diagnostics` permission). In the **Run settings** card choose the **Store**, switch **Deep check** on — it also queries live Google Analytics data and spends GA API quota — and press **Run diagnostics**. The same run over HTTP is `POST api/sales-rep/analytics-diagnostics?storeId=<store>&includeLiveData=true`. The **Checklist** card then shows one row per stage, each marked Passed / Warning / Failed / Skipped with a message and an expandable detail: | Stage | What it answers | | --- | --- | | Configuration | Is a property id set for this store? | | Credentials | Did Application Default Credentials resolve? | | API access | Does Google answer this property at all? | | Custom dimensions | Are the four user-scoped dimensions registered? | | Report compatibility | Can the dimensions and metrics the widgets need be queried together? | | Realtime data | Is the store sending events right now? | | Processed data | Is there data the Data API can already report on? | | Feature query | The real thing: a store-wide probe (`session_kind=self`, `is_sales_rep=false`, last 30 days, top 5) for search terms and browsed products | Use it before blaming the widgets — a red row here names the cause, where an empty widget does not. **Realtime data** passing while **Processed data** is still empty is the normal state right after setup; with **Deep check** off, the live stages come back as Skipped. ### What to expect while testing - **GA4 takes up to 24–48 hours to process events.** Empty widgets right after a search are the expected state, not a defect. Realtime sees the traffic at once; the Data API the widgets read does not. - **Reports are aggregates.** The finest time dimension is the hour, so "last searched" is an hour bucket, never the exact event time. - **One search action fires both `search` and `view_search_results`;** only `search` is counted, so a single search adds 1 to the term's count, not 2. - **A rep's own browsing never counts.** A rep is a member of every organization they serve, so their own searches, product views and sign-ins carry that organization; they are tagged `is_sales_rep=true` and left out. Generate test activity as a customer member, not as the rep. - **Coverage is a sample, not a record** — ad blockers and consent mode mean GA sees a subset of real activity. - **Turn Google Signals off** on a property used for per-customer reporting. With it on, GA4 suppresses rows for small cohorts, which is exactly the shape of a single-customer query. - A browse-history row links to the product only when the backend could resolve GA's `item_id` (which holds the product **code**) to a product; rows it could not resolve stay plain text by design. A code that several catalogs carry resolves to the product the store's catalog shows. ## References ### Jira-link: https://virtocommerce.atlassian.net/browse/VCST-5337 ### Artifact URL: https://vc3prerelease.blob.core.windows.net/packages/vc-theme-b2b-vue-2.61.0-pr-2458-3f0b-3f0b7284.zip <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes global GA4 tagging, login event timing, and sign-out identity clearing; misconfiguration affects all storefront analytics reporting, though scope is limited to the analytics module and new sales-rep UI. > > **Overview** > **Customer identity on GA4 events** and **Sales Rep activity surfaces** that consume that data are the two main threads. > > The Google Analytics module now sets **user-scoped custom dimensions** (`contact_id`, `organization_id`, `organization_name`, `session_kind`, `is_sales_rep`) via `gtag('set', 'user_properties', …)` on init and whenever the signed-in user changes, including explicit **null/anonymous** values on sign-out so stale identities do not stick. Identity is applied **before** the first `config` (and pushed to `dataLayer` ahead of GTM when configured). Successful **`login` events are deferred** across the post-sign-in reload using `sessionStorage` and sent once on the landing page under the new identity; failed logins still fire immediately. README and unit tests cover registration, ordering, and edge cases. > > On the read side, the sales-rep module adds **GraphQL operations** for `salesRepActivities`, `salesRepCustomerActivitySummary`, and customer insights (search terms / browsed products), plus UI such as **`activity-row`** (hour-bucket timestamps, deep links), **`customer-activity`** (summary + lazy-mounted Searches/Product views chips), and **`tracked-metric-hint`** for GA latency. Core menu links gain optional **`activeWhen`** and **`isMenuLinkActive`** so nav (e.g. My customers on a customer activity URL) can be active beyond vue-router’s record match. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f18143d. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: vk <vk@virtoway.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Aleksandra-Mitricheva <54628043+Aleksandra-Mitricheva@users.noreply.github.com>
Description
Teaches the module to read from Google Analytics 4, not only to tag. Until now it was write-only: it stored a measurement id / GTM container id per store and handed them to the storefront, which pushed events to Google; nothing could ask Google what those events said. This adds a generic, source-agnostic analytics read API — "give me the events matching these filters" — backed by the GA4 Data API (
runReport), plus a staged diagnostics endpoint that tells an operator exactly why a GA connection does not work.The API is deliberately domain-free: it speaks events, dimensions, filters and date ranges, not customers or sales reps. Consumers compose their own business meaning on top (the first consumer is
VirtoCommerce.SalesRep, which builds rep-facing search/browse insights from it).No database, no migrations, no changes to any existing endpoint, setting or behavior: the anonymous
GET api/googleanalytics/{storeId}settings endpoint is byte-identical and still carries only the three public tagging settings. Three new store-level settings (all non-public, none exposed through that anonymous endpoint), two new NuGet dependencies (the Google client libraries), and one project-level packaging change. The module version is unchanged from dev (4.1005.0).Companion PRs: VirtoCommerce/vc-module-sales-rep#15 and VirtoCommerce/vc-frontend#2458
New GraphQL queries and mutations
None — this module exposes no GraphQL surface, and this PR does not introduce one. The read API is consumed in-process through
IAnalyticsService; modules that need a GraphQL field build it on their own schema.Changes to existing GraphQL queries and mutations
None.
New REST endpoints
POST api/googleanalytics/{storeId}/diagnosticsgoogleanalytics:accessThe request body is optional; every field defaults, so
{}(or no body) runs a bare connectivity check:{ // user-scoped custom dimensions to verify, WITHOUT the customUser: prefix "userDimensionNames": ["contact_id", "organization_id", "session_kind"], "eventNames": ["search", "view_item", "login"], // events you expect to be collected "reports": [{ // report shapes to compatibility-check "name": "searchTerms", "dimensionNames": ["eventName", "dateHour", "searchTerm"], "metricName": "eventCount", "eventNames": ["search", "view_search_results"] }], "includeLiveData": true // false skips the two data stages (saves quota) }The response is a stable, ordered checklist —
{ "checks": [ { "stage", "status", "message", "detail" } ] }— withstatusone ofPassed | Warning | Failed | Skipped:configurationcredentialsapiAccessanalytics.readonlyscope; Data API not enabled in the named GCP project; quota project missing; caller lacks Viewer on the property; unknown property idcustomDimensionsreportCompatibilityrealtimeprocessedDataFailedatconfiguration,credentialsorapiAccessmarks the remaining stagesSkipped, so the shape never varies. Diagnostics bypasses the response cache and reads Google directly, so it always reports the live state. Empty results areWarning, notFailed— "no data yet" is a state, not a fault. Secrets never appear in the payload: the credential stage reports only the kind of credential in use.New public services / interfaces / methods
IAnalyticsService+AnalyticsServiceIsConfiguredAsync(storeId);SearchEventsAsync(criteria)→ rows, withTotalCount(rows) andTotalEventCount(the events those rows stand for — an hour-bucket row can hold several — taken in the same request from GA's metric total over every matching row, whatever page was asked for);GetEventSummariesAsync(criteria)→ one total + last-occurrence per requested event name, answered by one narrow read per name, all in one parallel round (at a bounded concurrency): a one-rowdate-sorted read whose response also carries the metric total, so the count and the last hour arrive together. A summary therefore waits for one read, never one read after another, which keeps its worst case at one request timeout; and it never reduces the hourly series — GA has nomax(dateHour), so fetching it would transfer one row per event name per hour to produce two numbers. Without event names, the names are found first (acount-mode read) and then read the same way. Any of those reads failing fails the whole summary. An earlier round isolated a failed read and kept the totals with a nullLastOccurredAt; that was reversed once reads began to throw, because a null there reads as "this event never happened" and a consumer presents it as fact — at the cost of one transient failure costing the summary until the failure entry expires, where before it cost a single field. Resolves settings per store and throws rather than answer a failure with an empty result. A read checks three things in order — the criteria (ArgumentException), then the store's configuration, then Google (AnalyticsExceptionfor both) — and only the third is cached, so a store configured a minute after a failed read reports at once instead of after the TTL. The exception names the store and the operation and nothing else; the property id and Google's own response stay in this module's log. A consumer catches it and decides how to degrade, which is what keeps "reporting is broken" distinguishable from "this customer did nothing".AnalyticsExceptionIAnalyticsDiagnosticsService+AnalyticsDiagnosticsServiceIAnalyticsSettingsResolver+AnalyticsSettingsResolverAnalyticsEventCriteriaBase.BypassCacheIAnalyticsDataSource+GoogleAnalyticsDataSourceprotected virtual, so reshaping one report is a subclass rather than a replacement. It is deliberately not a provider-abstraction boundary:AnalyticsDataQueryis GA4-shaped (a numeric property id, dimension names that ARE GA4apiNames) and a different source would need query fields this one has no place for,AnalyticsDiagnosticsServicechecks Google specifically, and both types live in.Data— so reading from somewhere else is a wider change than swapping this type, and the module does not claim otherwise.IGoogleAnalyticsReportClient+GoogleAnalyticsReportClientBetaAnalyticsDataClient(RunReport,RunRealtimeReport,CheckCompatibility,GetMetadata, credential validation); every call takes the deadline it must answer within. Swap it in DI to test against a fake without touching Google.AnalyticsEventSearchCriteria/AnalyticsEventSummaryCriteria(both: AnalyticsEventCriteriaBase),AnalyticsEvent,AnalyticsEventSearchResult,AnalyticsEventSummary,AnalyticsDimensionFilter,AnalyticsDiagnostics{Request,ReportShape,Check,Result}AbstractTypeFactory(the module previously registered none — extending these types is now possible).The criteria in one place.
StoreId,EventNames,DimensionFilters(AND across filters, OR within one),From/To(UTC), plus for searchesDimensionNames(extra dimensions to return per row),Take/Skip, andSortBy:SortBy = "date"(default) — one row per hour bucket, newest first.AnalyticsEvent.OccurredAtcarries the UTC hour-bucket start, never an event timestamp — GA reports are aggregates anddateHouris the finest dimension available. GA4 returnsdateHourin the property's reporting timezone and ships that zone in every response, so the bucket is converted to UTC on the way in rather than assumed; an unresolvable zone id leaves the bucket uncorrected and logs a warning. Known limit in the other direction:From/Toare still sent as UTC dates, so a range boundary can differ by up to a day from the property's calendar — the zone is only known from a response, so narrowing that would cost a metadata round trip per read.SortBy = "count"— GA-aggregated rows ordered by the metric, descending;OccurredAtisnullbecause a ranked total has no single time. Use this for "top N", the date mode for "most recent".Dimension names are passed without the
customUser:prefix; the data source adds it for the four known user-scoped names and strips it again on the way back, so callers never handle the prefix. GA's"(not set)"is normalized to an absent key rather than a literal.New constants for callers:
ModuleConstants.EventNames(the seven ecommerce/auth events the storefront sends),UserDimensions(contact_id,organization_id,is_sales_rep,session_kind— the names thecustomUser:prefix is applied to; the values each carries are the consumer's business, not this module's),Dimensions(searchTerm,itemId,itemName,itemListName,eventName,dateHour),SortBy,DiagnosticsStages,DiagnosticsStatuses.New protected methods (extensibility)
AnalyticsService.PrepareCriteria(...),ValidateCriteria(...),ResolveSettingsAsync(...),GetOrCreateAsync(...),ReadAsync(...),CreateReadException(...),WithNormalizedDates(...),CreateQuery(...),CreateSummariesAsync(...),GetEventNamesAsync(...),GetSummaryAsync(...),GetCacheTtl(...),ApplyCacheTtl(...)andFailureCacheTtl(default 60 s) — override any of the three steps (argument checks, configuration resolution, the call), query composition, how a summary finds its event names or reads one of them, what a failure is reported as, cache TTL, or how long a failure is remembered.GoogleAnalyticsDataSource.GetEventScopedRowsAsync(...)/GetItemScopedRowsAsync(...),BuildEventReportRequest(...)/BuildItemReportRequest(...),CreateRequest(...),AddDateHourAndExtraDimensions(...),AddOrderBy(...),BuildDimensionFilter(...),MapResponse(...),ParseMetricTotal(...),MapDimensionValue(...),MapDimensionName(...)/UnmapDimensionName(...),HasItemDimensions(...),ResolvePropertyTimeZone(...)— override any part of request building or row mapping without replacing the source.GoogleAnalyticsReportClient.CreateCallSettings(...),GetClientAsync(...),GetCredentialAsync(...),CreateClientAsync(...),ResolveCredentialAsync(...),GetApplicationDefaultCredentialAsync(...)— override how each call's deadline is applied, how the credential is obtained and scoped, or how the GA client is built, without replacing the client.AnalyticsDiagnosticsServiceexposes every stage as its ownprotected virtual(CheckConfigurationAsync,CheckCredentialsAsync,CheckApiAccessAsync,CheckCustomDimensions,CheckReportCompatibilityAsync,CheckRealtimeAsync,CheckProcessedDataAsync), plusMapApiAccessFailure(...)to add your own Google-error-to-advice mappings and theBuild*Request(...)helpers.Breaking changes
One published constructor gained a parameter; nothing was removed or renamed.
GoogleAnalyticsController's constructor takesIAnalyticsDiagnosticsServicefor the new diagnostics endpoint, so a project that subclasses the controller passes it tobase(...). Everything else in this PR is new. Some members changed within the PR's own new surface:GoogleAnalyticsDataSourceandAnalyticsSettingsResolvertake anILogger,MapDimensionValuetakes the resolvedTimeZoneInfo, everyIGoogleAnalyticsReportClientmethod takes the call's timeout,GetSummaryAsync/GetEventNamesAsyncreplaced the two summary helpersGetLastOccurredAtAsync/CreateSummaries, andUserDimensions.OrganizationNamewas dropped — the storefront no longer sends the organization's name, which can be a person's. That is free here because none of it has shipped, but worth naming for anyone building on a branch build. The read contract itself changed the same way:SearchEventsAsync/GetEventSummariesAsyncused to log and return an empty result on any failure and now throwAnalyticsException, so a consumer built against an earlier branch build must wrap its reads.No
[Obsolete]was introduced: nothing was deprecated.CopyLocalLockFileAssembliesis now enabled on the.Webproject. Without it the Google client assemblies never reach the platform's probing folder and every Data API call fails at runtime withFileNotFoundException: Google.Api.Gax. It makes the module package larger by the Google/gRPC dependency chain.module.ignoreis updated in the same change to exclude the 15 copied assemblies the platform or a dependency module already provides (VirtoCommerce.Platform.Core,Newtonsoft.Json,Serilog, theSystem.IO.Abstractionstrio,VirtoCommerce.StoreModule.Coreand the module cores behind it), so what ships is the Google/gRPC chain and this module's own three assemblies.New dependencies
Google.Analytics.Data.V1Beta2.0.0-beta10 — the GA4 Data API client (runReport,runRealtimeReport,checkCompatibility,getMetadata).beta10is the newest.Google.Apis.Auth1.73.0 — declared directly (it previously arrived only transitively) forGoogleCredential, whichGoogleAnalyticsReportClientresolves and scopes.No new module dependency: the manifest still declares only
VirtoCommerce.Store.Localization
The three new settings are surfaced in the admin UI, so each carries a title and description in all 10 shipped
locales (
de,en,es,fr,it,ja,pl,pt,ru,zh) undersettings.GoogleAnalytics4.DataApi.{PropertyId,CacheTtlMinutes,RequestTimeoutSeconds}. Nothing else in the module's localization fileschanged.
New settings
Three store-level settings under the existing Google Analytics 4 group, all
IsPublic = falseand therefore never served by the anonymous settings endpoint:GoogleAnalytics4.DataApi.PropertyIdG-XXXXXXXmeasurement id).GoogleAnalytics4.DataApi.CacheTtlMinutes0disables the cache for this module (every read goes to Google), and the platform's ownCaching:CacheEnabled=falseis honoured — both are useful when diagnosing stale reporting data.GoogleAnalytics4.DataApi.RequestTimeoutSecondsCredentials are Application Default Credentials only — there is deliberately no credential setting, so no key material is stored in the platform database or reachable through the settings API. ADC covers workload identity and the metadata server in a cluster,
GOOGLE_APPLICATION_CREDENTIALSpointing at a key file, andgcloud auth application-default loginlocally. The credential is scoped toanalytics.readonly— least privilege, and read-only by construction — and its principal needs Viewer on the GA4 property.gcloud auth application-default logindoes not carry the analytics scope, and a user credential ignoresCreateScoped, so every Data API call then failsPermissionDenied. Locally, log in with the scope:gcloud auth application-default login --scopes="https://www.googleapis.com/auth/analytics.readonly,https://www.googleapis.com/auth/cloud-platform,https://www.googleapis.com/auth/userinfo.email,openid"followed bygcloud auth application-default set-quota-project <project>.IsConfiguredAsyncis true when a property id is present — credentials are deliberately not part of the answer, because ADC supplies them out of band. Itsfalsetherefore means exactly one thing: the settings resolved and carry no property id. If they cannot be resolved at all it throws like a read, so an outage never reaches a consumer as "not configured".All three settings, the ADC prerequisite and the diagnostics endpoint are documented in
docs/index.md.References
QA-test:
Jira-link:
https://virtocommerce.atlassian.net/browse/VCST-5337
Artifact URL:
https://vc3prerelease.blob.core.windows.net/packages/VirtoCommerce.GoogleEcommerceAnalytics_4.1005.0-pr-61-5267.zip
Note
Medium Risk
Large new integration with Google Data API, ADC/credential setup, and caching semantics that affect quota and error handling; existing public tagging endpoints are untouched.
Overview
Adds read path for GA4 alongside existing tagging: in-process
IAnalyticsServicequeries the GA4 Data API (runReport) for event searches and per-event summaries, backed by new non-public store settings (numeric property id, report cache TTL, request timeout) and Application Default Credentials withanalytics.readonly— no credential settings in the database.Reads throw
AnalyticsException(and validate criteria withArgumentException) instead of returning empty results on misconfiguration or Google failures; responses are cached per store/query (including brief failure caching for quota protection).POST api/googleanalytics/{storeId}/diagnosticsruns a seven-stage checklist (config, credentials, API access, custom dimensions, report compatibility, realtime, processed data), gated bygoogleanalytics:access.Implementation adds Google client packages,
GoogleAnalyticsReportClient/GoogleAnalyticsDataSource, dimension-filter guards, item-scoped vs event-scoped report paths, and extensive tests; docs and admin localizations cover setup and failure behavior. Existing anonymous GET store settings and tagging behavior are unchanged;CopyLocalLockFileAssembliesandmodule.ignoreupdates ship the Google/gRPC dependencies.Reviewed by Cursor Bugbot for commit 60df472. Bugbot is set up for automated code reviews on this repo. Configure here.