Skip to content

[ENG-1607] Add Session Tag + Filtering#3735

Merged
chitalian merged 1 commit intomainfrom
charlie/eng-1620
May 13, 2025
Merged

[ENG-1607] Add Session Tag + Filtering#3735
chitalian merged 1 commit intomainfrom
charlie/eng-1620

Conversation

@hcharlie1201
Copy link
Contributor

@hcharlie1201 hcharlie1201 commented May 10, 2025

  • Have special filters when column is "tag" then we apply a where filter to only the ids that have the tags in it (open to suggestion of just looking for tags and table name)
  • Fixed added new copy function
  • Have a new table in Clickhouse for tags
  • run a subquery
  • small refactors by replacing the thumbs up with tags (request by QAWolf)

Notion:

Frontend:

Note

  • Currently in session view tables they don't see the tags as a column yet. Is they ask, then maybe we can provide, because currently I think we might have to do a join which is expensive.

Steps:

  • Once pr looks good and test, ill separate out into backend and frontend and merge backend + deploy first

@vercel
Copy link

vercel bot commented May 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
helicone ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 13, 2025 1:48am
helicone-bifrost ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 13, 2025 1:48am
helicone-eu 🛑 Canceled (Inspect) May 13, 2025 1:48am

@promptless
Copy link
Contributor

promptless bot commented May 10, 2025

📝 Documentation updates detected! You can review documentation updates here

@fumedev
Copy link

fumedev bot commented May 10, 2025

Summary

  • Introduces a new filtering mechanism for sessions based on tags in the database.
  • Adds endpoints to get and update session tags in the API.
  • Creates a new "tags" table in Clickhouse to store session tags.
  • Implements a subquery to filter sessions by tag.
  • Includes minor refactors and improvements to existing code.
  • Frontend updates include a new component for editing session tags.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this

@@ -0,0 +1,10 @@
CREATE TABLE IF NOT EXISTS default.tags (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add organization id here as the highest order on the cardinality to help the queries be faster. Also to check auth faster in the SessionManager


async getSessionTag(
sessionId: string
): Promise<Result<string | null, string>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no auth checks here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

own clarification: org check

const [formTag, setFormTag] = useState(getTag(id, type) || "");
const { setNotification } = useNotification();
const [open, setOpen] = useState(false);
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is the way zustand recommends to do it, but i feel like there has to be a better way LOL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a helper function to set it instead :D. Also better typing here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job. Less code >>>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dope you got the filterd to work

continue;
}

switch (true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, switches in typescript are not like Elixr or Rust.

They SUCK.

Rust match statements >>>>

but in typescript it kind of annoying.

If you wanna use a switch, I ask that you move it to a helper function and use returns so we can remove all breaks

}),
sessions_request_response_rmt:
easyKeyMappings<"sessions_request_response_rmt">({}),
easyKeyMappings<"sessions_request_response_rmt">({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dope you figured it out!

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR implements a comprehensive session tagging system to replace the existing thumbs up/down feedback mechanism, with support for both UI management and backend filtering capabilities.

  • New ClickHouse tags table added in schema_46_tags.sql using ReplacingMergeTree for deduplicating tagged entities
  • Introduced SessionTag component in web/components/templates/feedback/sessionTag.tsx with modal interface and state management via useTagStore
  • Changed latency calculation in web/lib/sessions/helpers.ts from time-span to cumulative trace latency
  • Added special tag filtering in valhalla/jawn/src/lib/shared/filters/filters.ts using subqueries to filter sessions by tag values
  • Expanded copy-packages.sh to copy entire common directory instead of just auth/server for better package sharing

26 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile

entity_id String,
tag String,
created_at DateTime DEFAULT now(),
PRIMARY KEY (organization_id, entity_type, entity_id, tag)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PRIMARY KEY (organization_id, entity_type, entity_id, tag)
PRIMARY KEY (organization_id, entity_type)

Primary keys are weird in clickhouse.

We want to have it as the highest order granuale that is ~8k rows

This will be sufficient

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-05-12 at 6 25 18 PM

Actually do we need primary key here? Since we're technically filtering on everything?

@chitalian chitalian merged commit 502dea2 into main May 13, 2025
6 of 9 checks passed
@chitalian chitalian deleted the charlie/eng-1620 branch May 13, 2025 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants