Refactoring HTTP downloader progress reporter to accept multiple observers #3542

ycombinator · 2023-10-05T15:29:33Z

What does this PR do?

Prior to this PR, the HTTP downloader's progressReporter would report download progress in Agent logs. This functionality was implemented entirely within the progressReporter code.

This PR refactors the progressReporter code to accept multiple observers and introduces a new observer, loggingProgressObserver, that logs the observed progress in Agent logs, preserving the behavior prior to this PR.

Why is it important?

This PR is purely a refactoring PR; it does not change any functionality. Functionally, the HTTP downloader's progress will continue to be reported in Agent logs.

However, in #3527, we will need the progressReporter to also report progress to another observer. This PR makes it easy to accomplish that in a clean manner. More context here: #3527 (comment)

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~I have made corresponding changes to the documentation~~
~~I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
~~I have added an entry in ./changelog/fragments using the changelog tool~~
~~I have added an integration test or an E2E test~~

Related issues

Relates to Track upgrade details #3527 (comment)

elasticmachine · 2023-10-05T15:29:36Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

elasticmachine · 2023-10-05T15:36:15Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-10-12T21:14:45.038+0000
Duration: 27 min 14 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6489
Skipped	59
Total	6548

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages.
run integration tests : Run the Elastic Agent Integration tests.
run end-to-end tests : Generate the packages and run the E2E Tests.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-10-05T16:20:19Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	98.78% (`81/82`)	👍
Files	67.224% (`201/299`)	👍 0.221
Classes	65.827% (`366/556`)	👍 0.185
Methods	53.06% (`1153/2173`)	👍 0.087
Lines	38.619% (`13163/34084`)	👍 0.041
Conditionals	100.0% (`0/0`)	💚

ycombinator · 2023-10-05T18:32:35Z

While working on this PR, I found a bug with the progress reporter. It was reporting progress just once (after a delay) instead of reporting it periodically. As such, this PR here is now blocked on the bugfix PR: #3548.

mergify · 2023-10-10T19:47:59Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b refactor-upgrade-download-progress-tracker upstream/refactor-upgrade-download-progress-tracker
git merge upstream/main
git push upstream refactor-upgrade-download-progress-tracker

michalpristas

just a very tiny comments, it looks good otherwise

michalpristas · 2023-10-12T12:11:46Z

internal/pkg/agent/application/upgrade/artifact/download/http/downloader.go

@@ -208,8 +206,9 @@ func (e *Downloader) downloadFile(ctx context.Context, artifactName, filename, f
 		}
 	}

+	lpObs := newLoggingProgressObserver(e.log, e.config.HTTPTransportSettings.Timeout)


wild name, i had to go up here to figure it what it means

Fixed in e9fc097.

michalpristas · 2023-10-12T12:15:29Z

internal/pkg/agent/application/upgrade/artifact/download/http/progress_reporter.go

+	length := dp.length
+	interval := dp.interval
+
+	go func() {


very unnecessary optimization from my side: no need to spin up goroutine or initiate tickers if progressObservers is []. as we're having at least log now, i consider this just a small optimization from component side of view.

Good point, will add, thanks!

Added in da06f8c and c81477f.

michalpristas · 2023-10-12T12:17:02Z

internal/pkg/agent/application/upgrade/artifact/download/http/progress_reporter.go

+		defer t.Stop()
+		for {
+			select {
+			case <-ctx.Done():


we're counting on external elements to cancel this. we could introduce Done chan internally which would be closed on ReportComplete or ReportFailed

What about the case where, for some reason, the consumer wants to cancel while the download is in progress (so before it's completed or failed)? I guess we could make it required as part of the interface's contract that either ReportComplete or ReportFailed MUST be called? Then we could safely remove the ctx from Report and handle cancellation as you suggested.

Implemented your suggestion in 67f80f6. Let me know what you think.

what about creating a subcontext that can be closed when the request is complete or cancelled (ofc we need to hold a ref to its cancellation function) ?

what about creating a subcontext that can be closed when the request is complete or cancelled (ofc we need to hold a ref to its cancellation function) ?

I like this pattern better as it doesn't require the consumer to remember to necessarily call either ReportComplete or ReportFailed. Basically, it gives the consumer two options on how to cancel: either cancel the ctx passed to Report or call ReportComplete/ReportFailed.

As far as implementation goes, we could do a subcontext or we use an internal done channel as implemented in 67f80f6. Is one implementation necessarily better than the other?

The context cancellation is more "abandon processing" and the done channel is the simple solution to unblock a go routine.

The context has the advantage of propagation through the subcontexts if the parent context is cancelled/closed/expires, the done channel has to be managed/closed explicitly.

Since you have already implemented the done channel you can keep that, I am just quite surprised to see that we only send an empty struct instead of closing the channel and removing the reference

Right, except I'm not seeing the benefit of using a subcontext over a done channel in this case, because (if I understood your suggestion correctly) we would do something like this with a subcontext:

create the subcontext with cancellation in the Report method ,

add a case for it to be done in the select in the Report method, replacing the current case for the ctx (parent context),

hold a reference to the cancellation function on the progressReporter struct,

and call this cancellation function from the ReportComplete and ReportFailed methods.

Whereas, using a done channel pattern, we:

create the done channel in the constructor

add a case for it to be done in the select in the Report method,

hold a reference to it on the progressReporter struct,

send an empty struct to the done channel or close it from the ReportComplete and ReportFailed methods.

So I'm not seeing much of a benefit in this case of using a subcontext instead of a done channel pattern. In particular, I don't see us taking advantage of the context propagation. Or am I missing something?

@pchila @michalpristas I updated the implementation such that there are now two ways to return from the goroutine inside Report:

Report is passed a context. When the context is done, the goroutine will return.

When the consumer calls ReportComplete or ReportFailed, we close an internal done channel, which will also cause the goroutine in Report to return.

michalpristas · 2023-10-12T12:20:03Z

internal/pkg/agent/application/upgrade/artifact/download/http/progress_reporter.go

+	return n, nil
+}
+
+func (dp *downloadProgressReporter) Report(ctx context.Context) {


make a comment that caller is responsible for cancelling context, otherwise we're leaking goroutine and ticker

Done in 89ca86b.

pchila

A few nits, nothing major.
Left a couple of questions...

pchila · 2023-10-12T12:20:22Z

internal/pkg/agent/application/upgrade/artifact/download/http/downloader.go

@@ -46,13 +44,13 @@ const (

 // Downloader is a downloader able to fetch artifacts from elastic.co web page.
 type Downloader struct {
-	log    progressLogger


Nit: I always prefer the logger as interfaces for mocking and loose coupling, instead of relying on a more concrete type

In general, I agree but I'm also a fan of YAGNI. We can easily change this back to an interface if/when we need it, i.e. have more than one implementation.

pchila · 2023-10-12T12:28:41Z

internal/pkg/agent/application/upgrade/artifact/download/http/downloader_test.go

@@ -253,21 +237,21 @@ func assertLogs(t *testing.T, logs []logMessage, minExpectedProgressLogs int, ex
 	// Verify that the first minExpectedProgressLogs messages are about the download progress (for the first file).
 	i := 0
 	for ; i < minExpectedProgressLogs; i++ {
-		assert.Equal(t, logs[i].record, expectedProgressMsg)
+		assert.Regexp(t, expectedProgressRegexp, logs[i].Message)


If there are other logs mixed in with the progress logs, this assert will fail the test. Is this intentional?

Good point. As things stand, the only other log that could get mixed up is this one:

elastic-agent/internal/pkg/agent/application/upgrade/artifact/download/http/downloader.go

Lines 102 to 104 in 359be4f

if err := os.Remove(path); err != nil {

e.log.Warnf("failed to cleanup %s: %v", path, err)

}

This log should never get triggered but I suppose in some strange circumstances the os.Remove could fail.

Any suggestions on how to make this more robust to only consider progress logs? Maybe the logs could first be filtered to only keep ones that match expectedProgressRegexp or expectedCompletedRegexp before running any assertions?

Filtering and then asserting length is surely one option. The exact semantics depend on what we are trying to assert here: from the code you are asserting that at least the first minExpectedProgressLogs log entries must be the ones you expect: this will break as soon as an extra log entry is in the mix (it may even be added in the future).

Did you mean to assert that among the collected log entries you have at least minExpectedProgressLogs expected logs ? In this case filter/count and then assert

Let me put up a commit showing what I meant by filtering and see what you think.

@pchila I implemented the filtering in a464077. Let me know what you think.

internal/pkg/agent/application/upgrade/artifact/download/http/progress_reporter.go

pchila · 2023-10-12T12:48:00Z

internal/pkg/agent/application/upgrade/artifact/download/http/progress_reporter.go

+		defer t.Stop()
+		for {
+			select {
+			case <-ctx.Done():


what about creating a subcontext that can be closed when the request is complete or cancelled (ofc we need to hold a ref to its cancellation function) ?

blakerouse

This looks good to me. I tried to find something to complain about, I was unsuccessful. ;-)

…vers

elastic-sonarqube · 2023-10-12T21:27:30Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

91.7% Coverage
0.0% Duplication

ycombinator added Team:Elastic-Agent Label for the Agent team backport-skip refactoring skip-changelog labels Oct 5, 2023

ycombinator requested a review from pchila October 5, 2023 15:29

ycombinator requested a review from a team as a code owner October 5, 2023 15:29

ycombinator requested a review from faec October 5, 2023 15:29

ycombinator mentioned this pull request Oct 5, 2023

Track upgrade details #3527

Merged

7 tasks

mergify bot assigned ycombinator Oct 5, 2023

ycombinator mentioned this pull request Oct 5, 2023

Replace timer with ticker in download progress reporter #3548

Merged

7 tasks

ycombinator marked this pull request as draft October 5, 2023 18:36

ycombinator removed request for faec and pchila October 5, 2023 18:36

ycombinator removed the Team:Elastic-Agent Label for the Agent team label Oct 5, 2023

ycombinator force-pushed the refactor-upgrade-download-progress-tracker branch 2 times, most recently from f20ba32 to 3335534 Compare October 5, 2023 19:29

ycombinator force-pushed the refactor-upgrade-download-progress-tracker branch from 3335534 to c7618c9 Compare October 10, 2023 20:48

ycombinator added the Team:Elastic-Agent Label for the Agent team label Oct 10, 2023

ycombinator marked this pull request as ready for review October 10, 2023 20:52

ycombinator added Team:Elastic-Agent Label for the Agent team and removed Team:Elastic-Agent Label for the Agent team labels Oct 10, 2023

pierrehilbert requested review from michalpristas, blakerouse and pchila October 11, 2023 06:23

ycombinator force-pushed the refactor-upgrade-download-progress-tracker branch from 22ddf67 to 9c04408 Compare October 12, 2023 12:06

michalpristas approved these changes Oct 12, 2023

View reviewed changes

michalpristas reviewed Oct 12, 2023

View reviewed changes

ycombinator requested a review from michalpristas October 12, 2023 12:49

pchila reviewed Oct 12, 2023

View reviewed changes

ycombinator requested a review from pchila October 12, 2023 13:11

blakerouse approved these changes Oct 12, 2023

View reviewed changes

ycombinator added 13 commits October 12, 2023 09:56

Refactoring HTTP downlader progress reporter to accept multiple obser…

56aa8ee

…vers

Improving names

5127fbf

Running mage fmt

b5e57f7

Fixing conflicts

bd072c2

Rename variable

5d0e1a5

Renaming receivers

1d7fedf

Better variable name

58b8f21

Add comment about Report callers needing to cancel context

a9e7b7d

Add optimization

1149d94

Remove context and handle cancellation internally instead

1fbea8c

More optimizations

5796d17

Add back context

5c87bc8

Make test more robust

ecc4201

ycombinator force-pushed the refactor-upgrade-download-progress-tracker branch from a464077 to ecc4201 Compare October 12, 2023 16:56

ycombinator changed the title ~~Refactoring HTTP downlader progress reporter to accept multiple observers~~ Refactoring HTTP downloader progress reporter to accept multiple observers Oct 12, 2023

Print logs if assertions fail

54efdb9

pchila approved these changes Oct 13, 2023

View reviewed changes

ycombinator merged commit 8284ce2 into elastic:main Oct 13, 2023
8 checks passed

ycombinator deleted the refactor-upgrade-download-progress-tracker branch October 13, 2023 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring HTTP downloader progress reporter to accept multiple observers #3542

Refactoring HTTP downloader progress reporter to accept multiple observers #3542

ycombinator commented Oct 5, 2023 •

edited

Loading

elasticmachine commented Oct 5, 2023

elasticmachine commented Oct 5, 2023 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented Oct 5, 2023 •

edited

Loading

ycombinator commented Oct 5, 2023

mergify bot commented Oct 10, 2023

michalpristas left a comment

michalpristas Oct 12, 2023

ycombinator Oct 12, 2023

michalpristas Oct 12, 2023

ycombinator Oct 12, 2023

ycombinator Oct 12, 2023 •

edited

Loading

michalpristas Oct 12, 2023

ycombinator Oct 12, 2023

ycombinator Oct 12, 2023

pchila Oct 12, 2023

ycombinator Oct 12, 2023

pchila Oct 12, 2023

ycombinator Oct 12, 2023 •

edited

Loading

ycombinator Oct 12, 2023

michalpristas Oct 12, 2023 •

edited

Loading

ycombinator Oct 12, 2023

pchila left a comment

pchila Oct 12, 2023

ycombinator Oct 12, 2023

pchila Oct 12, 2023

ycombinator Oct 12, 2023

pchila Oct 12, 2023

ycombinator Oct 12, 2023

ycombinator Oct 12, 2023

pchila Oct 12, 2023

blakerouse left a comment

elastic-sonarqube bot commented Oct 12, 2023

	if err := os.Remove(path); err != nil {
	e.log.Warnf("failed to cleanup %s: %v", path, err)
	}

Refactoring HTTP downloader progress reporter to accept multiple observers #3542

Refactoring HTTP downloader progress reporter to accept multiple observers #3542

Conversation

ycombinator commented Oct 5, 2023 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented Oct 5, 2023

elasticmachine commented Oct 5, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Oct 5, 2023 • edited Loading

🌐 Coverage report

ycombinator commented Oct 5, 2023

mergify bot commented Oct 10, 2023

michalpristas 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

ycombinator Oct 12, 2023 • edited Loading

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

ycombinator Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalpristas Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchila 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

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

blakerouse left a comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Oct 12, 2023

ycombinator commented Oct 5, 2023 •

edited

Loading

elasticmachine commented Oct 5, 2023 •

edited

Loading

elasticmachine commented Oct 5, 2023 •

edited

Loading

ycombinator Oct 12, 2023 •

edited

Loading

ycombinator Oct 12, 2023 •

edited

Loading

michalpristas Oct 12, 2023 •

edited

Loading