fix: pass groupbyTagSecondLevelKey through beacon-group mapping (website + mobile analyze) - #126
fix: pass groupbyTagSecondLevelKey through beacon-group mapping (website + mobile analyze)#126ppolezhaev wants to merge 1 commit into
Conversation
…ite + mobile) get_website_beacon_groups and get_mobile_app_beacon_groups rebuild the caller's group object keeping only groupbyTag and groupbyTagEntity, so groupbyTagSecondLevelKey is silently dropped. That makes it impossible to group key/value tags (e.g. beacon.meta / mobileBeacon.meta) by a specific sub-key: the API then falls back to enumerating the map's key names instead of splitting by the requested key's values, with no error or warning to the caller. The API models already support the field (WebsiteBeaconTagGroup and MobileAppBeaconTagGroup both declare groupbyTagSecondLevelKey), so this only forwards it: map the field in both modules (accepting the same camelCase variant handling as the existing group fields) and document it in the tool docstrings. Adds unit tests for both modules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: petrpolezhaev <petr.polezhaev@sixt.com>
NiyathaVS
left a comment
There was a problem hiding this comment.
Hey @ppolezhaev, you’ll need to update the docstrings exposed by the mobile_app and website smart routers. The docstrings from the smart routers are what get exposed to the LLM, and the LLM uses that information to construct the payload that is ultimately sent to the API. Currently, both routers are still exposing the old behavior, which doesn’t include groupbyTagSecondLevelKey. As a result, the LLM will continue generating payloads without this field, since the validation/logic added in the code itself isn’t visible to the LLM.
Could you please update the router docstrings to reflect the new behavior and include these changes? This will ensure that the LLM knows about the new field and includes it when constructing the payload.
|
|
||
| def test_build_beacon_groups_query_params_passes_second_level_key(self): | ||
| query_params = self.client._build_beacon_groups_query_params( | ||
| beacon_type="CUSTOM_EVENT", |
There was a problem hiding this comment.
beacon_type: "CUSTOM_EVENT" is not a valid beacon type, intended?
|
Thank you @NiyathaVS for your comment, that is also the first thing I noticed in the PR. But I have given it a bit more thought, and now lean towards more on not having this declared/surfaced in the tool's docstring:
So if we only scope this PR to just unblock callers (like @ppolezhaev) who already know the field and want to pass it, this PR as of now is sufficient for this scope/purpose without explicitly surfacing it in the tool docstring - but @NiyathaVS @sunjit10 , I appreciate your opinion and different arguments for decision on this one. If we do agree with my proposed scope, then this PR/fix itself looks right to me. I traced the path and the field |
Problem
get_website_beacon_groupsandget_mobile_app_beacon_groupsrebuild the caller'sgroupobject keeping onlygroupbyTagandgroupbyTagEntity—groupbyTagSecondLevelKeyis silently dropped (website:_map_group_fields; mobile: the inline mapping in_build_beacon_groups_query_params). That makes it impossible to group key/value tags (e.g.beacon.meta/mobileBeacon.meta) by a specific sub-key: the API then falls back to enumerating the map's key names instead of splitting by the requested key's values, with no error or warning to the caller.Repro
ineum('reportEvent', 'my_event', {meta: {flow: 'checkout'}})get_website_beacon_groupswithgroup: {"groupbyTag": "beacon.meta", "groupbyTagEntity": "NOT_APPLICABLE", "groupbyTagSecondLevelKey": "flow"}flow(checkout, …). Actual: a list of the meta key names — identical to omittinggroupbyTagSecondLevelKeyentirely.Fix
The API models already support the field — both
WebsiteBeaconTagGroupandMobileAppBeaconTagGroupdeclaregroupbyTagSecondLevelKey— so this change only forwards it: map the field in both modules (with the same camelCase-variant handling as the existing group fields) and document it in the tool docstrings.Tests
uv run testpasses (the only failure,TestVersionImport.test_version_fallback_on_exception, is pre-existing onmain).uv run ruff check .passes.Companion to #125 — together they make custom-event
metafully usable through the beacon analyze tools (values in list output, and grouping by sub-key).🤖 Generated with Claude Code