Skip to content

[Feature] [Platform] Storage Debug #1928

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

Merged
merged 1 commit into from
Jul 7, 2025
Merged

Conversation

ajanikow
Copy link
Collaborator

@ajanikow ajanikow commented Jul 4, 2025

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jul 4, 2025
@ajanikow ajanikow requested a review from Copilot July 4, 2025 14:45
Copilot

This comment was marked as outdated.

@ajanikow ajanikow force-pushed the feature/platform/storage_debug branch from 30243cb to 0b305d2 Compare July 4, 2025 17:04
@ajanikow ajanikow requested a review from Copilot July 4, 2025 17:04
@ajanikow ajanikow force-pushed the feature/platform/storage_debug branch from 0b305d2 to b0ae223 Compare July 4, 2025 17:05
Copilot

This comment was marked as outdated.

@ajanikow ajanikow requested a review from jwierzbo July 5, 2025 13:12
@ajanikow ajanikow force-pushed the feature/platform/storage_debug branch from b0ae223 to 745736a Compare July 7, 2025 11:06
@ajanikow ajanikow requested a review from Copilot July 7, 2025 11:07
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a debug‐package extension for the shutdown integration that collects local debug files and sends them to the Storage V2 service, updates context and shutdown handling to support cancellation hooks, and introduces a SendFile helper for streaming files.

  • Added a ContextKey type and shutdown‐cancel context helpers
  • Implemented the Debug shutdown extension (ExtensionShutdownV1Debug) and wired it into shutdown and profile generation
  • Introduced SendFile in Storage V2 helpers and updated CLI/docs for new debug flags

Reviewed Changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/util/context.go Added ContextKey type for context‐value keys
pkg/util/closer/close.go Fixed mutex usage in closeOnce.Close()
pkg/integrations/suite.go Introduced WithShutdownV1ContextCancelFunc/extractShutdownFunc
pkg/integrations/sidecar/extension.shutdown.v1.debug.go New debug extension for shutdown
pkg/integrations/shutdown_v1.go Plumbed debug config, flags, and handler changes
pkg/integrations/register.go Integrated shutdown debug into service registration
pkg/deployment/resources/arango_profiles.go Extended profile labels function to accept additional KV pairs
integrations/storage/v2/definition/helpers.go Added SendFile helper for streaming local files
docs/ Updated CLI flags and examples for shutdown debug
cmd/ Switched various Execute() calls to ExecuteContext(shutdown.Context())
Comments suppressed due to low confidence (2)

pkg/util/context.go:122

  • [nitpick] The new ContextKey type lacks a doc comment. Adding a brief comment explaining its intended use and conventions will improve readability and maintainability.
type ContextKey string

docs/integration-sidecar.md:104

  • [nitpick] The example label key storage.integration.profiles.arangodb.com/debug may not match the code-generated key from NewProfileIntegrationFeature. Please verify that the docs and the actual label format are aligned.
    storage.integration.profiles.arangodb.com/debug: "enabled"

Comment on lines 64 to +71
if labels == nil {
return nil
}

out := make(map[string]string)

for k, v := range labels {
out[k] = v
Copy link
Preview

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When labels is nil, the function returns nil and ignores any additional entries. Consider initializing the output map unconditionally so that additional key/value pairs are respected even if the base labels map is empty.

Suggested change
if labels == nil {
return nil
}
out := make(map[string]string)
for k, v := range labels {
out[k] = v
out := make(map[string]string)
if labels != nil {
for k, v := range labels {
out[k] = v
}

Copilot uses AI. Check for mistakes.

Comment on lines +45 to +48
if cerr := f.Close(); cerr != nil {
return nil, cerr
}

Copy link
Preview

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit f.Close() in the error block combined with the deferred f.Close() causes a redundant file close. Consider removing the manual close and relying on the defer, or otherwise consolidating close logic to avoid double-close scenarios.

Suggested change
if cerr := f.Close(); cerr != nil {
return nil, cerr
}

Copilot uses AI. Check for mistakes.

@ajanikow ajanikow force-pushed the feature/platform/storage_debug branch from 745736a to 94ff950 Compare July 7, 2025 12:36
@ajanikow ajanikow force-pushed the feature/platform/storage_debug branch from 94ff950 to 6dc2f11 Compare July 7, 2025 12:57
@ajanikow ajanikow merged commit 598b6f5 into master Jul 7, 2025
3 checks passed
@ajanikow ajanikow deleted the feature/platform/storage_debug branch July 7, 2025 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants