Added Atlas counter titus.executor.S3Uploader.uploadMB to report log … #151

rickkw · 2018-06-29T20:58:13Z

Report metrics on log file size uploaded in MB

coveralls · 2018-06-29T21:54:20Z

Pull Request Test Coverage Report for Build 1605

6 of 18 (33.33%) changed or added relevant lines in 5 files are covered.
373 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.6%) to 23.042%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
cmd/titus-executor/main.go	0	1	0.0%
cmd/titus-standalone/main.go	0	1	0.0%
executor/mock/jobrunner.go	0	1	0.0%
uploader/uploader.go	0	4	0.0%
uploader/s3.go	6	11	54.55%

Files with Coverage Reduction	New Missed Lines	%
executor/runtime/docker/docker.go	7	2.21%
executor/runtime/types/types.go	15	30.68%
executor/mock/jobrunner.go	36	0.0%
api/netflix/titus/titus_agent_api.pb.go	79	4.69%
api/netflix/titus/agent.pb.go	114	17.76%
api/netflix/titus/titus_base.pb.go	122	4.56%

Totals
Change from base Build 1566:	0.6%
Covered Lines:	2095
Relevant Lines:	9092

💛 - Coveralls

codecov · 2018-06-29T21:58:57Z

Codecov Report

Merging #151 into master will increase coverage by 13.19%.
The diff coverage is 56.25%.

@@             Coverage Diff             @@
##           master     #151       +/-   ##
===========================================
+ Coverage   20.69%   33.88%   +13.19%     
===========================================
  Files          61       61               
  Lines        7414     7174      -240     
===========================================
+ Hits         1534     2431      +897     
+ Misses       5690     4445     -1245     
- Partials      190      298      +108

Impacted Files	Coverage Δ
cmd/titus-standalone/main.go	`0% <0%> (ø)`	⬆️
cmd/titus-executor/main.go	`4.83% <0%> (ø)`	⬆️
executor/mock/jobrunner.go	`77.23% <100%> (+77.23%)`	⬆️
uploader/s3.go	`7.35% <55.55%> (+7.35%)`	⬆️
uploader/uploader.go	`50% <75%> (+23.61%)`	⬆️
api/netflix/titus/titus_agent_api.pb.go	`5.34% <0%> (ø)`	⬆️
api/netflix/titus/titus_base.pb.go	`4.95% <0%> (+1.21%)`	⬆️
filesystems/watcher.go	`62% <0%> (+3.71%)`	⬆️
config/config.go	`97% <0%> (+4.5%)`	⬆️
uploader/copy.go	`50% <0%> (+5%)`	⬆️
... and 13 more

corindwyer · 2018-06-29T22:37:50Z

uploader/s3.go

@@ -87,6 +90,11 @@ func (u *S3Uploader) Upload(ctx context.Context, local string, remote string, ct
 		}
 	}()

+	// Record number of bytes being uploaded.  At this point, file must exist.
+	if stat, err := os.Stat(local); err == nil {
+		u.metrics.Counter("titus.executor.S3Uploader.uploadMB", int(stat.Size()/1024/1024), nil)


I think this needs tags or the only information it will tell me is that an agent uploaded some megabytes to S3 during some time frame.

what tags can you think of? Filename may not be a good choice if it rotates and have a numerical or timestamp suffix. I assume the standard tags already includes container ID, etc.

I was just thinking container ID. Does the u.metrics.Counter method automatically add container information?

nf.node is usually set to the container's task ID from my understanding. I might suggest just asking the insights team all the the nf.* tags they associate with task metrics and use those.

sargun · 2018-07-02T16:17:36Z

uploader/s3.go

@@ -87,6 +90,11 @@ func (u *S3Uploader) Upload(ctx context.Context, local string, remote string, ct
 		}
 	}()

+	// Record number of bytes being uploaded.  At this point, file must exist.
+	if stat, err := os.Stat(local); err == nil {
+		u.metrics.Counter("titus.executor.S3Uploader.uploadMB", int(stat.Size()/1024/1024), nil)


nf.node is usually set to the container's task ID from my understanding. I might suggest just asking the insights team all the the nf.* tags they associate with task metrics and use those.

sargun · 2018-07-02T16:20:27Z

uploader/s3.go

@@ -87,6 +90,11 @@ func (u *S3Uploader) Upload(ctx context.Context, local string, remote string, ct
 		}
 	}()

+	// Record number of bytes being uploaded.  At this point, file must exist.
+	if stat, err := os.Stat(local); err == nil {


Don't use os.Stat here because then you wont catch stuff being uploaded via UploadPartOfFile.

You should move the code into uploadFile. I might see if there's a simple io.Reader wrapper to get callbacks as data is read.

sargun · 2018-07-02T20:39:35Z

uploader/s3.go

@@ -90,24 +93,43 @@ func (u *S3Uploader) Upload(ctx context.Context, local string, remote string, ct
 	return u.uploadFile(ctx, f, remote, contentType)
 }

+// CountingReader is a wrapper of io.Reader to count number of bytes read
+type CountingReader struct {


Why is CountingReader exposed outside of the package? It should be countingReader if it's not used outside of the package (equivalent to private in Java).

sargun · 2018-07-02T20:40:12Z

uploader/s3.go

+// CountingReader is a wrapper of io.Reader to count number of bytes read
+type CountingReader struct {
+	reader    io.Reader
+	BytesRead int


Might want to make this an int64?

sargun · 2018-07-02T20:48:01Z

uploader/s3.go

 // UploadFile writes a single file only to S3!
 func (u *S3Uploader) uploadFile(ctx context.Context, local io.Reader, remote string, contentType string) error {
 	u.log.Printf("Attempting to upload file from: %s to: %s", local, path.Join(u.bucketName, remote))
 	if contentType == "" {
 		contentType = defaultS3ContentType
 	}

+	// wrap input io.Reader with a counting reader
+	reader := &CountingReader{local, 0}


In general, it's better to initialize with explicit field names. Also, by default in Go, uninitialized fields have default values. For ints, this is 0.

sargun · 2018-07-02T20:48:14Z

uploader/s3.go

 	result, err := u.s3Uploader.UploadWithContext(ctx, &s3manager.UploadInput{
 		ACL:         aws.String(defaultS3ACL),
 		ContentType: aws.String(contentType),
 		Bucket:      aws.String(u.bucketName),
 		Key:         aws.String(remote),
-		Body:        local,
+		Body:        io.Reader(reader),


Is this cast neccessary here?

sargun · 2018-07-02T20:48:29Z

uploader/s3_test.go

+	"testing"
+)
+
+func TestCountingReader_Read(t *testing.T) {


TestCountingReader_Read -> TestCountingReaderRead

sargun · 2018-07-02T20:51:20Z

uploader/s3_test.go

+
+	buffer := make([]byte, 10)
+	for {
+		_, err := countingReader.Read(buffer)


You can use:

https://golang.org/pkg/io/ioutil/#ReadAll

https://golang.org/pkg/io/#Copy with ioutil.discard

There's also a library which makes asserting on these errors easier: https://github.com/stretchr/testify

You can do something like assert.Equal(t, len(data), n)

sargun · 2018-07-02T22:00:42Z

uploader/s3_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestCountingReader_Read(t *testing.T) {


Huh, I thought there was a comment to remove this underscore?

sargun

Please rewrite / squash your commits

rickkw requested review from fabiokung, corindwyer and sargun June 29, 2018 20:58

corindwyer suggested changes Jun 29, 2018

View reviewed changes

sargun suggested changes Jul 2, 2018

View reviewed changes

sargun approved these changes Jul 2, 2018

View reviewed changes

corindwyer approved these changes Jul 2, 2018

View reviewed changes

sargun approved these changes Jul 2, 2018

View reviewed changes

fabiokung approved these changes Jul 3, 2018

View reviewed changes

Added Atlas counter titus.executor.S3Uploader.successfullyUploadedBytes

e75b6b1

rickkw force-pushed the TITUS-895_publish_metrics_log_upload_in_MB branch from aed7d28 to e75b6b1 Compare July 9, 2018 19:57

rickkw merged commit 3b03700 into master Jul 9, 2018

rickkw deleted the TITUS-895_publish_metrics_log_upload_in_MB branch July 12, 2018 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Atlas counter titus.executor.S3Uploader.uploadMB to report log … #151

Added Atlas counter titus.executor.S3Uploader.uploadMB to report log … #151

rickkw commented Jun 29, 2018

coveralls commented Jun 29, 2018 •

edited

Loading

codecov bot commented Jun 29, 2018 •

edited

Loading

corindwyer Jun 29, 2018

rickkw Jun 29, 2018

corindwyer Jun 29, 2018

sargun Jul 2, 2018

sargun Jul 2, 2018

sargun Jul 2, 2018

rickkw Jul 2, 2018

sargun Jul 2, 2018

sargun Jul 2, 2018

sargun Jul 2, 2018

sargun Jul 2, 2018

sargun Jul 2, 2018

sargun Jul 2, 2018

sargun Jul 2, 2018

sargun left a comment

Added Atlas counter titus.executor.S3Uploader.uploadMB to report log … #151

Added Atlas counter titus.executor.S3Uploader.uploadMB to report log … #151

Conversation

rickkw commented Jun 29, 2018

coveralls commented Jun 29, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1605

💛 - Coveralls

codecov bot commented Jun 29, 2018 • edited Loading

Codecov Report

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

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

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

sargun left a comment

Choose a reason for hiding this comment

coveralls commented Jun 29, 2018 •

edited

Loading

codecov bot commented Jun 29, 2018 •

edited

Loading