- 
                Notifications
    
You must be signed in to change notification settings  - Fork 12
 
refactor(metric provider): use explore style time ranges #2603
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
base: main
Are you sure you want to change the base?
Conversation
7ce4d8b    to
    f214e76      
    Compare
  
    | datasource?: QueryDatasource | ||
| maxTimeframe?: TimeframeKeys | ||
| overrideTimeframe?: Timeframe | ||
| tz?: string | 
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.
Removed tz as a prop as it can just be provided with the time range?
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.
Good call.
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'd like to review.
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.
(Haven't looked at ui-apps yet.)
I think as long as some of the queryTime code remains, the tests that exercise it should remain as well.  I think that means it's feasible to remove a lot of the tests, but there are some absolute time tests in there that are probably still relevant?
| datasource?: QueryDatasource | ||
| maxTimeframe?: TimeframeKeys | ||
| overrideTimeframe?: Timeframe | ||
| tz?: string | 
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.
Good call.
| expect(new TimeseriesQueryTime(getTimePeriod(TimeframeKeys.PREVIOUS_MONTH)).startDate().getHours()).toEqual(0) | ||
| }) | ||
| 
               | 
          ||
| it('custom timeframe daily', () => { | 
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.
This test, at least, should probably stay here...
| }) | ||
| }) | ||
| 
               | 
          ||
| describe('non-timeseries queries with custom timeframes', () => { | 
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.
This test should probably remain.
| expect(deltaQuery.granularitySeconds()).toBe(unaryQuery.granularitySeconds()) | ||
| }) | ||
| 
               | 
          ||
| it('determines a correct trend query across multiple transitions', () => { | 
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.
This test should probably remain.
| } | ||
| return relativePeriod | ||
| const overrideTimeRange: Ref<TimeRangeV4> = computed(() => { | 
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.
This no longer needs to be a computed -- you can just reference the variable directly.
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.
done
f214e76    to
    ce4ebb0      
    Compare
  
    BREAKING CHANGE: metric provider props updated to accept explore style timerange
3228f5f    to
    3c83a89      
    Compare
  
    
          Preview components from this PR in consuming applicationIn consuming application project install preview versions of shared packages generated by this PR:  | 
    
| import { ref } from 'vue' | ||
| import type { ExploreResultV4 } from '@kong-ui-public/analytics-utilities' | ||
| import type { ExploreResultV4, | ||
| Timeframe } from '@kong-ui-public/analytics-utilities' | 
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.
This seems like it shouldn't need the Timeframe type any more.
Summary
BREAKING CHANGE: metric provider props updated to accept explore style time range