-
Notifications
You must be signed in to change notification settings - Fork 178
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
Fixing returning NaN when rate metrics change / reset counter #121
Changes from 3 commits
dd343be
4973439
e8b5e7c
79c1d04
a2da50b
e37403e
5ce19c8
6a641fc
eb4528b
07fdb71
e367622
8e07f80
1c19271
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import * as clickhouse from '..'; | ||
import { describe, beforeEach, jest, it, expect } from '@jest/globals'; | ||
|
||
describe('clickhouse', () => { | ||
beforeEach(() => { | ||
|
@@ -42,4 +43,29 @@ describe('clickhouse', () => { | |
expect(clickhouse.client.insert).toHaveBeenCalledTimes(2); | ||
expect.assertions(2); | ||
}); | ||
|
||
it('getMetricsChart', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would be awesome to have a bit more descriptive of a test case name |
||
jest | ||
.spyOn(clickhouse.client, 'query') | ||
.mockResolvedValueOnce({ json: () => Promise.resolve({}) } as any); | ||
|
||
await clickhouse.getMetricsChart({ | ||
aggFn: clickhouse.AggFn.AvgRate, | ||
dataType: 'Sum', | ||
endTime: Date.now(), | ||
granularity: clickhouse.Granularity.OneHour, | ||
name: 'test', | ||
q: '', | ||
startTime: Date.now() - 1000 * 60 * 60 * 24, | ||
teamId: 'test', | ||
}); | ||
|
||
expect(clickhouse.client.query).toHaveBeenCalledTimes(1); | ||
expect(clickhouse.client.query).toHaveBeenCalledWith( | ||
expect.objectContaining({ | ||
jaggederest marked this conversation as resolved.
Show resolved
Hide resolved
|
||
format: 'JSON', | ||
query: expect.stringContaining('isNaN(rate) = 0'), | ||
}), | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -167,18 +167,18 @@ | |
}); | ||
|
||
export const getLogStreamTableName = ( | ||
version: number | undefined | null, | ||
teamId: string, | ||
) => `default.${TableName.LogStream}`; | ||
|
||
export const buildTeamLogStreamWhereCondition = ( | ||
version: number | undefined | null, | ||
teamId: string, | ||
) => SqlString.raw('(1 = 1)'); | ||
|
||
export const buildLogStreamAdditionalFilters = ( | ||
version: number | undefined | null, | ||
teamId: string, | ||
) => SettingsMap.from({}); | ||
|
||
export const healthCheck = () => client.ping(); | ||
|
@@ -415,7 +415,7 @@ | |
// TODO: support since, until | ||
export const fetchMetricsPropertyTypeMappings = | ||
(intervalSecs: number) => | ||
async (tableVersion: number | undefined, teamId: string) => { | ||
Check warning on line 418 in packages/api/src/clickhouse/index.ts GitHub Actions / lint
|
||
const tableName = `default.${TableName.Metric}`; | ||
const query = SqlString.format( | ||
` | ||
|
@@ -704,6 +704,7 @@ | |
startTime: number; // unix in ms | ||
teamId: string; | ||
}) => { | ||
await redisClient.connect(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for this. redis client should be connected when server starts |
||
const tableName = `default.${TableName.Metric}`; | ||
const propertyTypeMappingsModel = await buildMetricsPropertyTypeMappingsModel( | ||
undefined, // default version | ||
|
@@ -803,6 +804,8 @@ | |
( | ||
? | ||
) | ||
WHERE | ||
isNaN(rate) = 0 | ||
`.trim(), | ||
[SqlString.raw(sumMetricSource)], | ||
); | ||
|
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.
I don't think we need to import these from globals. did you hit any issues on local ?