Skip to content
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

refactor: To ensure telemetry is only enabled when both conditions are met. #393

Closed
wants to merge 5 commits into from
Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions init/retina/main_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
package main

import (
"flag"

"github.com/microsoft/retina/pkg/bpf"
"github.com/microsoft/retina/pkg/config"
"github.com/microsoft/retina/pkg/log"
"github.com/microsoft/retina/pkg/telemetry"
"go.uber.org/zap"
Expand All @@ -21,23 +24,31 @@ var (
func main() {
// Initialize logger
opts := log.GetDefaultLogOpts()
zl, err := log.SetupZapLogger(opts)
if err != nil {
panic(err)
}

// Define a command-line flag "config" with a default value and parse the flags, then create a logger with a name and version.
configPath := flag.String("config", "/retina/config/config.yaml", "path to the config file")
Copy link
Contributor

Choose a reason for hiding this comment

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

@Carpe-Wang looks like you need to pass in the path to the config via arguments to the init-container as well. Please refer to how we are currently doing it for the agent's main container
image

Copy link
Author

Choose a reason for hiding this comment

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

image

I added the following section to the relevant file:

args:
  - --config=/retina/config/config.yaml

Does this meet your expectation?"

Copy link
Contributor

Choose a reason for hiding this comment

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

that should work

Copy link
Author

Choose a reason for hiding this comment

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

okay, I had push the newest commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

oops i forgot to mention, you will also have to mount the path the config file like we are doing with the main container. Thanks for the prompt response!

Copy link
Author

Choose a reason for hiding this comment

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

@nddq I change the file according to your comment. Please review it.

flag.Parse()
l := zl.Named("init-retina").With(zap.String("version", version))

// Load configuration
cfg, err := config.GetConfig(*configPath)
if err != nil {
l.Fatal("Failed to get config", zap.Error(err))
}

// Enable telemetry if applicationInsightsID is provided
if applicationInsightsID != "" {
if applicationInsightsID != "" && cfg.EnableTelemetry {
opts.EnableTelemetry = true
opts.ApplicationInsightsID = applicationInsightsID
// Initialize application insights
telemetry.InitAppInsights(applicationInsightsID, version)
defer telemetry.ShutdownAppInsights()
defer telemetry.TrackPanic()
}

zl, err := log.SetupZapLogger(opts)
if err != nil {
panic(err)
}
l := zl.Named("init-retina").With(zap.String("version", version))

// Setup BPF
bpf.Setup(l)
}