Skip to content

Fix metrics for newer models and with new trace types#70

Open
RyanFrench wants to merge 5 commits intoawslabs:mainfrom
RyanFrench:main
Open

Fix metrics for newer models and with new trace types#70
RyanFrench wants to merge 5 commits intoawslabs:mainfrom
RyanFrench:main

Conversation

@RyanFrench
Copy link
Copy Markdown
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Copy Markdown
Contributor

@ottokruse ottokruse left a comment

Choose a reason for hiding this comment

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

Should probably add a unit test for your new code that handles JSON in markdown blocks.

On the cycles. Why do you want that a dimension on the cycle? It works but be aware that every unique dimension counts in pricing as a new custom metric. So let's say you have 40 cycles, you are doing 40x the costs of the custom metric you already have.

@RyanFrench
Copy link
Copy Markdown
Contributor Author

Yes, I will add some unit tests for the new functionality. Primarily I was finding that newer models are ignoring the request to just output the raw JSON, which breaks the built-in metrics.

The cycles dimension can likely be removed, but in the current implementation the latency metric outputs warnings on almost every invocation about the inclusion of an unrecognised trace type. This is both confusing and worrying to users, who have no control over this. We either need to suppress this warning specifically for the cycles trace type, or add it to the list of dimensions like we do with the current trace types.

@ottokruse
Copy link
Copy Markdown
Contributor

but in the current implementation the latency metric outputs warnings on almost every invocation about the inclusion of an unrecognised trace type

That warning is just overly annoying then, I'd vote to remove it

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