Uh oh!
There was an error while loading. Please reload this page.
[FEATURE] add trace heatmap chart plugin - #724
Conversation
c40e966 to
139f2eaCompareb33097f to
cad2d04CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
e90da96 to
30c30f1Compare30c30f1 to
69d4258Compare69d4258 to
ec0a324Compare| func MustGetWorkspaces(dirPath string) []string { | ||
| excludedWorkspaces := []string{"e2e"} | ||
| excludedWorkspaces := []string{"e2e", "traceheatmapchart"} |
There was a problem hiding this comment.
At the moment this plugin does not include any Go files. Therefore, the pipeline throws an exception. For now, we can exclude the plugin from this part of the pipeline.
There was a problem hiding this comment.
not sure why the plugin should not include go files. At least the sdk related files should exist.
There was a problem hiding this comment.
we don't need these files as they are included in the repo root.
There was a problem hiding this comment.
we don't need these files as they are included in the repo root.
There was a problem hiding this comment.
we don't need these files as they are included in the repo root.
There was a problem hiding this comment.
we don't need these files as they are included in the repo root.
There was a problem hiding this comment.
we don't need these files as they are included in the repo root.
| @@ -0,0 +1,30 @@ | |||
| module github.com/perses/plugins/prometheus | |||
There was a problem hiding this comment.
| module github.com/perses/plugins/prometheus | |
| module github.com/perses/plugins/traceheatmapchart |
| @@ -0,0 +1,17 @@ | |||
| module: "github.com/perses/traceheatmapchart@v0" | |||
There was a problem hiding this comment.
| module: "github.com/perses/traceheatmapchart@v0" | |
| module: "github.com/perses/plugins/traceheatmapchart@v0" |
| @@ -0,0 +1,63 @@ | |||
| { | |||
| "name": "@perses/traceheatmap-chart-plugin", | |||
There was a problem hiding this comment.
| "name": "@perses/traceheatmap-chart-plugin", | |
| "name": "@perses-dev/traceheatmap-chart-plugin", |
| })(), | ||
| }); | ||
| return `<div style=" |
There was a problem hiding this comment.
this HTML is invalid, was this ran to check?
There was a problem hiding this comment.
Surprisingly the HTML works just fine although it has some syntax issue, funny! Let me check.
I also found out this can fall under the XSS attack! Let's see if I can improve it.
| scale: 'hourly', | ||
| getBuckets: (start, end, local) => { | ||
| const generateHourlyLabels = (ts: number) => { | ||
| const formatter = new Intl.DateTimeFormat('en-US', { hour: 'numeric' }); |
There was a problem hiding this comment.
can we avoid to hardcode the en-US locale?
| elementId: generateId(column, row, 'min'), | ||
| value: ((): string | undefined => { | ||
| const indexes = bucketToFlatResultsMap[column]?.[row]; | ||
| if (!indexes) return undefined; |
There was a problem hiding this comment.
since this does not guard against empty arrays, what would be displayed?
There was a problem hiding this comment.
If I understood your point right,
That should be fine to return undefined. When the Tooltip html is being built the undefined values are scaped by the filter
returnbucketDetails.filter((i)=>i.value)There was a problem hiding this comment.
What I meant is that indexes can be [] which is thuthy
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| export { getPluginModule } from './getPluginModule'; |
There was a problem hiding this comment.
We are missing component exports here.
| "dev": "rsbuild dev", | ||
| "build": "npm run build-mf && concurrently \"npm:build:*\"", | ||
| "build-mf": "rsbuild build", | ||
| "build:cjs": "swc ./src -d dist/lib/cjs --strip-leading-paths --config-file .cjs.swcrc", |
There was a problem hiding this comment.
not sure why are we re defining the cjs.swcrc if there is one at the root that can be re used, like other plugins already do.
| resultsStartTimes: Array<Pick<TraceSearchResult, 'startTimeUnixMs' | 'durationMs'>>, | ||
| local?: string | ||
| ): TraceHeatMapTimeBucket => { | ||
| const startTimeStamp = min(...resultsStartTimes.map((i) => i.startTimeUnixMs)); |
There was a problem hiding this comment.
This could create a stack overflow with many traces, the min and max functions don't take arrays, they are expanded as variadic parameters, but the V8's has limits on how many parameters you can add. It will be better to use a plain loop and calculate the max and min at the same time.
862d55b to
62c94d1Compare| const indexes = bucketToFlatResultsMap[column]?.[row]; | ||
| if (!indexes?.length) return undefined; | ||
| return ( | ||
| max(...indexes.filter((idx) => flatResults[idx] !== undefined).map((idx) => flatResults[idx]!.durationMs)) + '' |
There was a problem hiding this comment.
This still doing a sort over a large amount of variadic params
62c94d1 to
2a897cdCompareSigned-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Mahmoud Shahrokni <seyedmahmoud.shahrokni@amadeus.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com> Signed-off-by: Seyed Mahmoud Shahrokni <39291137+shahrokni@users.noreply.github.com>
2a897cd to
f114a88Compare| export const escapeHtml = (value: string): string => { | ||
| return value | ||
| .replaceAll('&', '&') | ||
| .replaceAll('<', '&alt;') |
There was a problem hiding this comment.
| .replaceAll('<','&alt;') | |
| .replaceAll('<','<') |
| }); | ||
| return `<div style=" | ||
| id=${generateId(column, row, 'container')} |
There was a problem hiding this comment.
Why is this id inside the css? This seems to be error prone, is there a more maintable way to build this HTML?
There was a problem hiding this comment.
There are also no tests around this function, so we are blind on the issues.
| export type FlatQueryResults = TraceSearchResult[]; | ||
| export const getHeatmapChartData = (flatQueryResults: FlatQueryResults, base: ExponentialBase): HeatmapChartData => { |
There was a problem hiding this comment.
it seems the tests are too shallow, they are not actually testing how this function groups the data. For example, what happens with traces that have 0 duration?
| minBucketCount = bucketCount; | ||
| } | ||
| bucketToFlatResultsMap[timeBucketIndex]![durationBucketIndex]!.push(i); |
There was a problem hiding this comment.
We should avoid using type assertions with ! what if duration bucket is empty?
| if (i === buckets.length) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Why do we need this if we are using a for loop with a condition?
| if (i === buckets.length) { | ||
| break; | ||
| } | ||
| if (buckets[i]! > buckets[i + 1]!) { |
There was a problem hiding this comment.
should this compare the start field?
| describe('escapeHtml', () => { | ||
| [ | ||
| { ch: '&', substitute: '&' }, | ||
| { ch: '<', substitute: '&alt;' }, |
There was a problem hiding this comment.
| {ch: '<',substitute: '&alt;'}, | |
| {ch: '<',substitute: '<'}, |
| const { | ||
| queryResults, | ||
| spec: { | ||
| bucketSettings: { base }, |
There was a problem hiding this comment.
can we guarantee bucketSettings is always there?
| ); | ||
| } | ||
| const option: EChartsOption = { |
There was a problem hiding this comment.
This is built on every render and might cause unnecessary re configurations
| getBuckets: (start: number, end: number, local?: string): TraceHeatMapTimeBucket => { | ||
| /* optimized using a js closure to avoid creating Intl.DateTimeFormat in every iteration */ | ||
| const generateMonthlyLabel = (ts: number) => { | ||
| const formatter = new Intl.DateTimeFormat(local, { month: 'long' }); |
There was a problem hiding this comment.
This is created on every iteration as the closure is inside the generateMonthlyLabel function. If reuse is needed or optimization it should be inside the getBuckets function.
constformatter=newIntl.DateTimeFormat(local,{month: 'long'});constgenerateMonthlyLabel=(ts: number): string=>{returnformatter.format(ts);};
Relates to perses/perses#3942
This is the bare minimum
trace heat map chart. All it needs is traces and the exponential base (default 2) to bucketize the traces according to their time and duration.The x-axis and y-axis represent the time and duration buckets respectively. Both buckets are generated dynamically.
The following picture shows a base 2 bucketing.
It also supports base 10. Choosing a proper base may vary by different cases.
Demo
Same queries and traces, different bases
Next Steps?
Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes