feat: add metrics for debuginfo #1704

ksankeerth · 2022-09-12T04:04:39Z

Hi Parca team,
I tried to fix #1275. Not sure I have covered everything mentioned in #1275. Please check and let me know if anything else to be added in this PR.

I have added metrics for..

errors and attempts for uploading debuginfo
errors and attempts for validation
time taken for uploading debuginfo, updating metdata, and, exists check

I'm not sure what exactly I have to do for below metrics. Will you be able to guide me?

Debuginfo Client metrics (will be exposed on Agent)

Debuginfod Metrics

Fixes #1275

Signed-off-by: shankeerthan-kasilingam shankeerthan1995@gmail.com

1. errors and attempts for uploading debuginfo 2. errors and attempts for updating metadata 3. time taken for uploading debuginfo, updating metdata, and, exists check Signed-off-by: shankeerthan-kasilingam <shankeerthan1995@gmail.com>

CLAassistant · 2022-09-12T04:04:43Z

All committers have signed the CLA.

kakkoyun

Looking good. Thanks for handling this.

I think we can be more consistent with the variable naming.

And as far as I can tell, we don't increment error metrics every error returns. Let's make sure we cover everything.

You might also want to consider extracting validation logic to the helper function fro ease of instrumenting.

kakkoyun · 2022-09-12T08:48:27Z

pkg/debuginfo/store.go

+		metadataUpdateDuration:       metadataUpdateDuration,
+		validationAttemptsTotal:      validationAttemptsTotal,
+		validationErrorsTotal:        *validationErrorsTotal,
+		uploadDebugInfoAttemptsTotal: uploadDebugInfoAttemptsTotal,


Suggested change

uploadDebugInfoAttemptsTotal: uploadDebugInfoAttemptsTotal,

debugInfoUploadAttemptsTotal: uploadDebugInfoAttemptsTotal,

Thanks for the suggestion. I'll make the changes.

kakkoyun · 2022-09-12T08:48:55Z

pkg/debuginfo/store.go

+		validationAttemptsTotal:      validationAttemptsTotal,
+		validationErrorsTotal:        *validationErrorsTotal,
+		uploadDebugInfoAttemptsTotal: uploadDebugInfoAttemptsTotal,
+		uploadDebugInfoErrorsTotal:   *uploadDebugInfoErrorsTotal,


Suggested change

uploadDebugInfoErrorsTotal: *uploadDebugInfoErrorsTotal,

debugInfoUploadErrorsTotal: *uploadDebugInfoErrorsTotal,

+1. will make the changes

kakkoyun · 2022-09-12T08:49:05Z

pkg/debuginfo/store.go

+		validationErrorsTotal:        *validationErrorsTotal,
+		uploadDebugInfoAttemptsTotal: uploadDebugInfoAttemptsTotal,
+		uploadDebugInfoErrorsTotal:   *uploadDebugInfoErrorsTotal,
+		uploadDebugInfoDuration:      uploadDebugInfoDuration,


Suggested change

uploadDebugInfoDuration: uploadDebugInfoDuration,

debugInfoUploadDuration: uploadDebugInfoDuration,

will make the changes

kakkoyun · 2022-09-12T08:58:53Z

pkg/debuginfo/store.go

@@ -253,7 +329,9 @@ func (s *Store) upload(ctx context.Context, buildID, hash string, r io.Reader) e

 	// At this point we know that we received a better version of the debug information file,
 	// so let the client upload it.
-
+	defer func(begin time.Time) {


I don't think this is semantically correct. There are lots of other operations happening in the rest of the method.

yes. thanks for correcting it. I think I cannot use defer here. I'll update the PR

kakkoyun · 2022-09-12T08:59:44Z

pkg/debuginfo/store.go

+
+	validationAttemptsTotal      prometheus.Counter
+	validationErrorsTotal        prometheus.CounterVec
+	metadataUpdateDuration       prometheus.Histogram


We should encapsulate this in metadata types and files rather than here.

A small doubt on this. I think We can move metadataUpdateDuration to metadata.go and encapsulate inside ObjectStoreMetadata struct. I'm not sure this comment for validationAttemptsTotal and validationErrorsTotal. Currently validation metrics are around this

parca/pkg/debuginfo/store.go

Line 302 in 573a48d

if err := elfutils.ValidateFile(objFile); err != nil {

Could you please clarify this?

Maybe we don't need those at all. Considering we'll always try to validate these files, maybe just having the duration is enough. What do you think?

@ksankeerth, I think we can get rid of additional validation attempts and error metrics. And add additional reason ("validation") to the actual action metrics (e.g upload).

nice. It's clear now. I'll update the PR

ksankeerth · 2022-09-14T02:24:34Z

Looking good. Thanks for handling this.

I think we can be more consistent with the variable naming.

And as far as I can tell, we don't increment error metrics every error returns. Let's make sure we cover everything.

You might also want to consider extracting validation logic to the helper function fro ease of instrumenting.

Thanks for reviewing the PR. I'll check the comments and update the PR

ksankeerth · 2022-09-14T03:17:22Z

And as far as I can tell, we don't increment error metrics every error returns.

Just some clarifications needed on this(sorry if it's a dump question).

parca/pkg/debuginfo/store.go

Line 85 in 573a48d

validationErrorsTotal prometheus.CounterVec

parca/pkg/debuginfo/store.go

Line 88 in 573a48d

uploadDebugInfoErrorsTotal prometheus.CounterVec

Two error metrics are used here. I increase the error metrics with reason before return if it's related to the metrics we measure. Is this correct? or in any places did i increase error metrics irrelevantly? (may be this location

parca/pkg/debuginfo/store.go

Line 321 in 573a48d

s.validationErrorsTotal.WithLabelValues("has_dwarf_object_file").Inc()

)

ksankeerth · 2022-09-14T03:20:23Z

You might also want to consider extracting validation logic to the helper function fro ease of instrumenting

If I'm going to extract validation logic to a function, Can I extract the below code inside a function in store.go?

parca/pkg/debuginfo/store.go

Lines 302 to 327 in 573a48d

    
           if err := elfutils.ValidateFile(objFile); err != nil { 
        
           	// Failed to validate. Mark the file as corrupted, and let the client try to upload it again. 
        
           	if err := s.metadata.MarkAsCorrupted(ctx, buildID); err != nil { 
        
           		level.Warn(s.logger).Log("msg", "failed to update metadata as corrupted", "err", err) 
        
           	} 
        
           	level.Error(s.logger).Log("msg", "failed to validate object file", "buildid", buildID) 
        
           	s.validationErrorsTotal.WithLabelValues("validate_object_file").Inc() 
        
           	// Client will retry. 
        
           	return status.Error(codes.Internal, err.Error()) 
        
           } 
        
           // Valid. 
        
           f, err := elf.Open(objFile) 
        
           if err != nil { 
        
           	level.Debug(s.logger).Log("msg", "failed to open object file", "err", err) 
        
           } else { 
        
           	hasDWARF, err := elfutils.HasDWARF(f) 
        
           	if err != nil { 
        
           		level.Debug(s.logger).Log("msg", "failed to check for DWARF", "err", err) 
        
           		s.validationErrorsTotal.WithLabelValues("has_dwarf_object_file").Inc() 
        
           	} 
        
           	f.Close() 
        
           	if hasDWARF { 
        
           		return status.Error(codes.AlreadyExists, "debuginfo already exists") 
        
           	} 
        
           }

kakkoyun · 2022-09-14T08:20:47Z

@ksankeerth, I think we can get rid of additional validation attempts and error metrics. And add additional reason ("validation") to the actual action metrics (e.g upload).

kakkoyun · 2022-09-14T08:22:07Z

You might also want to consider extracting validation logic to the helper function fro ease of instrumenting

If I'm going to extract validation logic to a function, Can I extract the below code inside a function in store.go?

parca/pkg/debuginfo/store.go

Lines 302 to 327 in 573a48d

if err := elfutils.ValidateFile(objFile); err != nil {

// Failed to validate. Mark the file as corrupted, and let the client try to upload it again.

if err := s.metadata.MarkAsCorrupted(ctx, buildID); err != nil {

level.Warn(s.logger).Log("msg", "failed to update metadata as corrupted", "err", err)

}

level.Error(s.logger).Log("msg", "failed to validate object file", "buildid", buildID)

s.validationErrorsTotal.WithLabelValues("validate_object_file").Inc()

// Client will retry.

return status.Error(codes.Internal, err.Error())

}

// Valid.

f, err := elf.Open(objFile)

if err != nil {

level.Debug(s.logger).Log("msg", "failed to open object file", "err", err)

} else {

hasDWARF, err := elfutils.HasDWARF(f)

if err != nil {

level.Debug(s.logger).Log("msg", "failed to check for DWARF", "err", err)

s.validationErrorsTotal.WithLabelValues("has_dwarf_object_file").Inc()

}

f.Close()

if hasDWARF {

return status.Error(codes.AlreadyExists, "debuginfo already exists")

}

}

You can extract it. I would prefer the metadata update logic outside the function on the call site, though.

Signed-off-by: shankeerthan-kasilingam <shankeerthan1995@gmail.com>

ksankeerth · 2022-09-15T05:20:40Z

@kakkoyun I have done following changes

changed the name of metrics as per your suggestions.
removed metrics for validationAttempts and validationErrors (since metrics were removed, i didn't extract metadata update logic to a separate function)

metadataUpdateDuration was moved to metadata.go and used metaData.UploadFinishedAt and metaData.UploadStartedAt for the metrics.
Seems race condition can happen

parca/pkg/debuginfo/metadata.go

Lines 179 to 182 in aa3772c

    
           // There's a small window where a race could happen. 
        
           if metaData.State == MetadataStateUploaded { 
        
           	return nil 
        
           }

. I didn't update metadataUpdateDuration metrics in this case.

Please check the new changes.Thanks

feat: add metrics for debuginfo

573a48d

1. errors and attempts for uploading debuginfo 2. errors and attempts for updating metadata 3. time taken for uploading debuginfo, updating metdata, and, exists check Signed-off-by: shankeerthan-kasilingam <shankeerthan1995@gmail.com>

ksankeerth requested review from a team as code owners September 12, 2022 04:04

kakkoyun reviewed Sep 12, 2022

View reviewed changes

fix code review comments

aa3772c

Signed-off-by: shankeerthan-kasilingam <shankeerthan1995@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add metrics for debuginfo #1704

feat: add metrics for debuginfo #1704

ksankeerth commented Sep 12, 2022 •

edited

CLAassistant commented Sep 12, 2022 •

edited

kakkoyun left a comment

kakkoyun Sep 12, 2022

kakkoyun Sep 12, 2022

ksankeerth Sep 14, 2022

kakkoyun Sep 12, 2022

ksankeerth Sep 14, 2022 •

edited

kakkoyun Sep 12, 2022

ksankeerth Sep 14, 2022

kakkoyun Sep 12, 2022

ksankeerth Sep 14, 2022 •

edited

kakkoyun Sep 12, 2022

ksankeerth Sep 14, 2022 •

edited

kakkoyun Sep 14, 2022

ksankeerth Sep 15, 2022

ksankeerth commented Sep 14, 2022

ksankeerth commented Sep 14, 2022

ksankeerth commented Sep 14, 2022

kakkoyun commented Sep 14, 2022

kakkoyun commented Sep 14, 2022

ksankeerth commented Sep 15, 2022

	uploadDebugInfoAttemptsTotal: uploadDebugInfoAttemptsTotal,
	debugInfoUploadAttemptsTotal: uploadDebugInfoAttemptsTotal,

	uploadDebugInfoErrorsTotal: *uploadDebugInfoErrorsTotal,
	debugInfoUploadErrorsTotal: *uploadDebugInfoErrorsTotal,

	uploadDebugInfoDuration: uploadDebugInfoDuration,
	debugInfoUploadDuration: uploadDebugInfoDuration,

feat: add metrics for debuginfo #1704

Are you sure you want to change the base?

feat: add metrics for debuginfo #1704

Conversation

ksankeerth commented Sep 12, 2022 • edited

CLAassistant commented Sep 12, 2022 • edited

kakkoyun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksankeerth Sep 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksankeerth Sep 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksankeerth Sep 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksankeerth commented Sep 14, 2022

ksankeerth commented Sep 14, 2022

ksankeerth commented Sep 14, 2022

kakkoyun commented Sep 14, 2022

kakkoyun commented Sep 14, 2022

ksankeerth commented Sep 15, 2022

ksankeerth commented Sep 12, 2022 •

edited

CLAassistant commented Sep 12, 2022 •

edited

ksankeerth Sep 14, 2022 •

edited

ksankeerth Sep 14, 2022 •

edited

ksankeerth Sep 14, 2022 •

edited