[7.7] [Uptime] Update uptime ml job id to limit to 64 char #64397
Conversation
shahzad31
added
the
Team:Uptime - DEPRECATED
|
Pinging @elastic/uptime (Team:uptime) |
shahzad31
changed the title
| import { getMLJobId } from '../ml_anomaly'; | ||
|
|
||
| describe('ML Anomaly API', () => { | ||
| it('it generates a valid ML job ID', async () => { |
There was a problem hiding this comment.
Can we break these out into different test cases? In an ideal world we'd do one it per test.
In some situations, where setup is complex and slow (like functional tests) breaking this rule makes sense. For simple fast pure functions like this one it really cleans up the code to break the tests up more.
So, you'd have a single block like:
it("should truncate long monitor IDs", () => {
expect(getMLJobId(longAndWeirdMonitorId)).toHaveLength(64)
}There was a problem hiding this comment.
i have updated the test.
| // ML Job ID can't be greater than 64 length, so will be substring it, and hope | ||
| // At such big length, there is minimum chance of having duplicate monitor id | ||
| // Subtracting ML_JOB_ID constant and _ char as well | ||
| if ((lowerCaseMonitorId + ML_JOB_ID + 1).length > 64) { |
There was a problem hiding this comment.
This is somewhat awkward in that in accounts for the extra _ we add. It feels like this function should just add the _ to the end itself. Is there a reason not to?
There was a problem hiding this comment.
yes, it's an awkward code, i have further tried simplifing it.
|
@elasticmachine merge upstream |
|
The code looks good, but running this branch, I'm not sure why, I can't ML to detect any anomalies. I'd like to figure out why that's the case before merging this. |
@andrewvc let me try producing Anomaly. On another thought should we increase the time from 24h to bit longer period in the past for records to be processed? |
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
|
@andrewvc i can see anomalies in this branch as well |
Backport the following PR:
#64394