-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: statefulsets implementation for k8s infra monitoring #6630
feat: statefulsets implementation for k8s infra monitoring #6630
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
b5d57e1
to
ba944d0
Compare
186f768
to
6d441f3
Compare
c89ffab
to
79cae2d
Compare
a294614
to
6855009
Compare
2edb730
to
579c7cb
Compare
579c7cb
to
22a33e3
Compare
fc0db62
to
16e60e4
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
❌ Changes requested. Reviewed everything up to 16e60e4 in 2 minutes and 13 seconds
More details
- Looked at
5046
lines of code in33
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:62
- Draft comment:
The conversion ofminTime
andmaxTime
to milliseconds seems incorrect. Use1000000
instead of1000000000
for the conversion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
- The division by 1000000000 converts nanoseconds to seconds (1 billion nanoseconds = 1 second). 2. The division by 1000000 would convert to milliseconds instead. 3. Looking at the code context, these values are used for time ranges and filtering, where seconds are a more appropriate unit than milliseconds. 4. The conversion is used consistently throughout the file.
I could be wrong about the intended time unit for the API or UI components. There might be documentation or other code that expects milliseconds.
The consistent use throughout the file and the fact that seconds are a common unit for time ranges in monitoring UIs suggests the current implementation is correct. If milliseconds were required, it would likely cause obvious issues that would have been caught.
The current conversion from nanoseconds to seconds using division by 1000000000 appears to be correct. The comment suggesting to use 1000000 instead would result in milliseconds, which does not seem to be the intended unit.
2. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/Metrics/StatefulSetMetrics.tsx:58
- Draft comment:
Using the entirepayload
object as part of thequeryKey
can lead to unnecessary re-fetches. Consider using a stable key or a subset of the payload that uniquely identifies the query. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment raises a valid React Query performance concern. Using mutable objects in queryKey can cause unnecessary re-renders. However, without seeing the payload structure from getStatefulSetQueryPayload(), it's hard to know if there's a better subset of data we could use. The current implementation is not necessarily wrong - it will work correctly, just maybe not optimally.
I don't have visibility into what fields are actually in the payload object. The current implementation might already be using the minimal necessary data.
While the concern is valid in theory, without being able to see the payload structure, we can't make a confident recommendation about what specific fields should be used instead.
The comment should be deleted because while it raises a valid concern, we don't have enough context to make a specific, actionable recommendation about how to improve the query key.
3. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/Traces/StatefulSetTraces.tsx:54
- Draft comment:
Theoffset
state is initialized but never updated. Ensure that the offset is updated appropriately for pagination to work correctly. - Reason this comment was not posted:
Comment did not seem useful.
4. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:428
- Draft comment:
Avoid using inline styles. Consider using CSS classes or styled components instead. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.styles.scss:4
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values for consistency in design and theming. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.styles.scss:43
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values for consistency in design and theming. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.styles.scss:184
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values for consistency in design and theming. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_EpyVFsLcVrxZKFbM
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
open={!!statefulSet} | ||
style={{ | ||
overscrollBehavior: 'contain', | ||
background: isDarkMode ? Color.BG_INK_400 : Color.BG_VANILLA_100, |
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.
Avoid using inline styles. Consider using CSS classes or styled components instead.
…ing-k8s-stateful-sets
Summary
Implement the statefulSets entity in Infra Monitoring
Related Issues / PR's
N/A
Screenshots
Affected Areas and Manually Tested Areas
Infra Monitoring section
Important
Implement Kubernetes StatefulSets monitoring in the infrastructure monitoring section with new API, UI components, and query handling.
getK8sStatefulSetsList
function ingetsK8sStatefulSetsList.ts
to fetch StatefulSets data.K8sStatefulSetsList
component inK8sStatefulSetsList.tsx
for displaying StatefulSets list with filtering and sorting.StatefulSetDetails
component inStatefulSetDetails.tsx
for detailed view of a StatefulSet, including metrics, logs, events, and traces.K8sStatefulSetsList.styles.scss
andStatefulSetDetails.styles.scss
.useGetK8sStatefulSetsList
hook inuseGetK8sStatefulSetsList.ts
for querying StatefulSets data.reactQueryKeys.ts
withGET_STATEFULSET_LIST
key.constants.ts
for StatefulSets.This description was created by for 16e60e4. It will automatically update as commits are pushed.