#88 Umami.Net: impossible to specify unique_id / DistinctId / Id#90
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #88 by adding support for passing an Umami distinct/unique identifier through the .NET client so users can be consistently identified across sessions.
Changes:
- Adds a new
distinctIdparameter toIdentify/IdentifyAndDecodeflows and forwards it intoUmamiPayload. - Introduces a new payload property (
Id) and ensuresPayloadServicepropagates it during payload population. - Updates a background-sender test call site to use a named argument due to the new parameter position.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Umami.Net/UmamiClient.cs | Adds distinctId support to identify APIs and forwards it into the payload. |
| Umami.Net/UmamiBackgroundSender.cs | Adds distinctId support for background identify/session identify enqueueing. |
| Umami.Net/PayloadService.cs | Propagates the new payload identifier property when populating payloads. |
| Umami.Net/Models/UmamiPayload.cs | Adds a new payload property (Id) intended to represent the distinct/unique identifier. |
| Umami.Net.Test/UmamiBackgroundSender_Tests.cs | Adjusts a call site to account for the updated method signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (payload.SessionId != null) | ||
| newPayload.SessionId = payload.SessionId; | ||
|
|
||
| if (payload.Id != null) | ||
| newPayload.Id = payload.Id; | ||
|
|
||
| newPayload.UserAgent = payload.UserAgent ?? DefaultUserAgent; | ||
|
|
There was a problem hiding this comment.
PayloadService.PopulateFromPayload now propagates payload.Id, but there’s no test coverage validating that this propagation works end-to-end (i.e., a provided distinct/unique id survives population and is sent). Consider adding a test that sets UmamiPayload.Id (or the renamed property) and verifies it is present in the final request JSON.
| public async Task<HttpResponseMessage> Identify(string? email = null, string? username = null, | ||
| string? sessionId = null, string? userId = null, UmamiEventData? eventData = null) | ||
| string? sessionId = null, string? userId = null, string? distinctId = null, UmamiEventData? eventData = null) | ||
| { |
There was a problem hiding this comment.
Adding the new optional parameter distinctId before eventData is a source/binary breaking change for any callers using positional arguments (e.g., Identify(..., eventData) will no longer compile). To maintain backward compatibility, keep the existing parameter order and add an overload (or move distinctId to the end) so existing positional call sites remain valid.
| string? distinctId = null, | ||
| UmamiEventData? eventData = null) |
There was a problem hiding this comment.
IdentifyAndDecode(string sessionId, ...) now inserts distinctId before eventData, which breaks positional callers that previously passed eventData as the next argument. Consider preserving the original signature and introducing a new overload that includes distinctId, or move distinctId to the end to avoid a breaking API change.
| string? distinctId = null, | |
| UmamiEventData? eventData = null) | |
| UmamiEventData? eventData = null, | |
| string? distinctId = null) |
| string? sessionId = null, string? userId = null, string? distinctId = null, UmamiEventData? eventData = null, | ||
| bool useDefaultUserAgent = false) |
There was a problem hiding this comment.
The new distinctId optional parameter is inserted before eventData, which is a breaking change for callers that pass eventData positionally (it will now try to bind to distinctId). Prefer keeping the existing parameter order and adding an overload (or placing distinctId at the end) to avoid breaking consumers.
| string? sessionId = null, string? userId = null, string? distinctId = null, UmamiEventData? eventData = null, | |
| bool useDefaultUserAgent = false) | |
| string? sessionId = null, string? userId = null, UmamiEventData? eventData = null, | |
| bool useDefaultUserAgent = false, string? distinctId = null) |
| public async Task IdentifySession(string sessionId, string? distinctId = null, UmamiEventData? eventData = null, | ||
| bool useDefaultUserAgent = false) | ||
| { |
There was a problem hiding this comment.
IdentifySession adds distinctId as the second parameter, which breaks existing positional calls like IdentifySession(sessionId, eventData) (the second argument no longer matches). To avoid a breaking change, keep the original signature and add an overload accepting distinctId, or move distinctId to the end after eventData/useDefaultUserAgent.
| public async Task IdentifySession(string sessionId, string? distinctId = null, UmamiEventData? eventData = null, | |
| bool useDefaultUserAgent = false) | |
| { | |
| public Task IdentifySession(string sessionId, UmamiEventData? eventData = null, | |
| bool useDefaultUserAgent = false) | |
| { | |
| // Preserve backward-compatible signature and forward to the extended overload | |
| return IdentifySession(sessionId, null, eventData, useDefaultUserAgent); | |
| } | |
| public async Task IdentifySession(string sessionId, string? distinctId, UmamiEventData? eventData = null, | |
| bool useDefaultUserAgent = false) | |
| { |
| public string? SessionId { get; set; } | ||
|
|
||
| public string? Id { get; set; } | ||
|
|
There was a problem hiding this comment.
UmamiPayload.Id is very generic and doesn’t communicate that it represents Umami’s distinct/unique identifier (issue #88). Consider renaming this to something explicit like DistinctId/UniqueId and mapping it to the expected JSON field name via [JsonPropertyName("id")] (or the exact key Umami expects), so the public API is self-describing without relying on the global naming policy.
| var payload = new UmamiPayload | ||
| { | ||
| SessionId = sessionId, | ||
| Id = distinctId, | ||
| Data = emailData | ||
| }; |
There was a problem hiding this comment.
New behavior: distinctId is now forwarded into the payload (Id = distinctId), but there are no assertions in the test suite that this value is actually serialized/sent when provided. Add/extend tests (client and/or background sender) to pass a non-null distinctId and assert it appears in the outgoing payload, so regressions are caught.
scottgal
left a comment
There was a problem hiding this comment.
Thanks for this PR @Shaddix — the feature is needed and the implementation is clean. A few items to address before merging:
Required Changes
-
Add
[JsonPropertyName("id")]toUmamiPayload.Id(UmamiPayload.cs)
WhileLowerCaseNamingPolicyproduces the correct"id"today, other properties (e.g.IpAddress→[JsonPropertyName("ip")]) use explicit attributes. Please add one for consistency and safety:[JsonPropertyName("id")] public string? Id { get; set; }
-
Add
distinctIdtoUmamiClient.IdentifySession(UmamiClient.cs:201)
UmamiBackgroundSender.IdentifySessionhas the newdistinctIdparameter, butUmamiClient.IdentifySessiondoes not — this is an inconsistency in the public API surface.
Suggestions (optional)
- Add a test that verifies
distinctIdflows through to the serialized payload'sidfield - Consider length validation — Umami docs state the
idfield has a 50-character limit - The
Track/TrackPageViewmethods don't exposedistinctIdas a convenience parameter (users can setIdon a rawUmamiPayload). This is fine for now but worth considering in a follow-up.
Overall this is a well-scoped, clean change — just needs the consistency fixes above. 👍
|
everything is done here, could you please check @scottgal ? |
|
It'd be great if you approve or comment on what needs to be changed :) |
|
+1 would be great to get this merged in. |
|
Thanks all, merged. I need to do a bigger pass / (if anone fancies it) on the new playback features too. I don't currently have time to maintain this. |
|
Will be in Umami.net 1.0.1 |
|
Thanks Scott. I've just started using Umami so alas my knowledge of replays isnt there yet! looking forward to 1.0.1 👍 |
|
(looks like the publish fell over though) |
|
Just fixed it, will be around shortly :) |
#88