Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor for message action bar and add metrics for alerting summary #304
Refactor for message action bar and add metrics for alerting summary #304
Changes from 44 commits
337dfd6
7938e5f
82c0fd4
19a5749
15f3d2a
578d8b8
876b68a
931ceaf
2e25985
21e2278
662923b
3ba3482
1956d49
14f9030
d247983
f799e15
d2278f1
f2516bb
d0f227d
a0e1a89
ae41d29
600f9f0
5f68dfa
07a5cff
5f27ef3
77ac476
4002c9b
15d44c3
14d6d0b
d94e244
cde988b
72aeb88
370b817
9eea26d
a8191e5
a35d147
53f7c4f
1d37a61
61da316
e7c2f7f
a56973a
9fafeae
2cd22d1
b665a42
b8bbda1
e99cfd9
c209f93
0bb77d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does metric name have length restriction? If this is the case, you might need to add some protection check in util function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I do not really know any specific restriction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use full name
alertSummary
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we record
generated
metric, i think the metric type could be METRIC_TYPE.COUNT.if we want to record
click
metric, it should not care the result of agent execution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I'll change it to METRIC_TYPE.COUNT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems weird to pass insight related status to Trace Icon and its related action function.
How about add a new parameter for the purpose of extending the MessageActions with customize buttons configuration? For this case, insight button should be passed in as a extending actions. You can think about which actions should be internal ones, and which should be extending ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like that the only difference between insight and trace is just the click function, so I think adding a new button is a little redundent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly. insight button needs to show info "Insight With RAG" when moving your cursor over it. And what if you wants to have both trace button and insight button, or need other customized buttons like insight? Overall, they should be 2 different buttons.
It could be an enhancement since this PR has been merged.