-
Notifications
You must be signed in to change notification settings - Fork 6
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
Telemetry correction #55
Conversation
Signed-off-by: Raziel Cohen <rcohen@armosec.io>
…r-time-scope Signed-off-by: Raziel Cohen <rcohen@armosec.io>
Summary:
|
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 think we should spend some time together and I'll show you what we expect from telemetry, as it is not just error handling
} | ||
} | ||
} | ||
logger.L().Ctx(ctx).Info("stop monitor on container - after monitoring time", helpers.String("container ID", contEvent.GetContainerID()), helpers.String("container name", contEvent.GetContainerName()), helpers.String("k8s resources", contEvent.GetK8SWorkloadID()), helpers.Error(err)) |
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.
we don't attach logs above Warning to spans, please remove .Ctx(ctx) from the line
@@ -160,10 +176,13 @@ func (ch *ContainerHandler) startRelevancyProcess(contEvent v1.ContainerEventDat | |||
err = ch.startTimer(watchedContainer, contEvent.GetContainerID()) | |||
if err != nil { | |||
if errors.Is(err, droppedEventsError) { | |||
logger.L().Ctx(context.GetBackgroundContext()).Warning("container monitoring got drop events - we may miss some realtime data", helpers.String("container ID", contEvent.GetContainerID()), helpers.String("container name", contEvent.GetContainerName()), helpers.String("k8s resources", contEvent.GetK8SWorkloadID()), helpers.Error(err)) | |||
ctx, span := otel.Tracer("").Start(ctx, "dropped events.", trace.WithAttributes(attribute.String("containerID", contEvent.GetContainerID()), attribute.String("container workload", contEvent.GetK8SWorkloadID()))) |
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.
don't create a span just for the error, they should be created when you enter a part of your code that you want to trace all the time (aka. record time spent there and compare with other spans)
Signed-off-by: Raziel Cohen <rcohen@armosec.io>
Signed-off-by: Raziel Cohen <rcohen@armosec.io>
Summary:
|
@rcohencyberarmor I corrected some things myself, PTAL |
Summary:
|
@@ -26,7 +26,7 @@ func (aggregator *Aggregator) collectDataFromContainerAccumulator(errChan chan e | |||
for { | |||
newEvent := <-aggregator.aggregationDataChan | |||
if newEvent.GetEventCMD() == accumulator.DropEventOccurred { | |||
// aggregator.StopAggregate() |
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 removed it by purpose, the management decided to continue listen on container when we got dropped events.
main.go
Outdated
@@ -14,6 +14,9 @@ import ( | |||
) | |||
|
|||
func main() { | |||
context.SetBackgroundContext() |
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.
it must be after the config parser, so we can get account ID etc..
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.
ok then don't try to attach warnings to the ctx without span
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
fff3e93
to
68b73f2
Compare
Summary:
|
@@ -26,7 +26,7 @@ func (aggregator *Aggregator) collectDataFromContainerAccumulator(errChan chan e | |||
for { | |||
newEvent := <-aggregator.aggregationDataChan | |||
if newEvent.GetEventCMD() == accumulator.DropEventOccurred { | |||
aggregator.StopAggregate() | |||
// aggregator.StopAggregate() |
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.
why did you stop accumulating here?
@@ -117,11 +123,9 @@ func (ch *ContainerHandler) startTimer(watchedContainer watchedContainerData, co | |||
} | |||
case err = <-watchedContainer.syncChannel[StepEventAggregator]: | |||
if err.Error() == accumulator.DropEventOccurred { | |||
watchedContainer.snifferTicker.Stop() |
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.
was it intended to be removed?
main.go
Outdated
@@ -14,6 +14,9 @@ import ( | |||
) | |||
|
|||
func main() { | |||
context.SetBackgroundContext() |
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.
ok then don't try to attach warnings to the ctx without span
Overview