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

Upgrade prometheus library #28023

Closed
wants to merge 0 commits into from
Closed

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Sep 21, 2021

What does this PR do?

This PR updates the github.com/prometheus/prometheus and makes the proper LICENCE overrides for the dependencies:

go get github.com/prometheus/prometheus@v1.8.2-0.20210701133801-b0944590a1c9
make notice

This is a prerequisite for #27269

Detailed licence checks for libs:

  1. github.com/ajstarks/svgo -> CC-BY-4.0
  2. github.com/bmizerany/pat -> from README.md
  3. github.com/cactus/go-statsd-client/statsd -> MIT
  4. github.com/dnaeon/go-vcr -> BSD-2-Clause
  5. github.com/golang/freetype -> GPL-2.0
  6. github.com/hudl/fargo -> [MIT] from README.md
  7. github.com/influxdata/line-protocol -> MIT
  8. github.com/lightstep/lightstep-tracer-common/golang/gogo -> MIT
  9. github.com/pascaldekloe/goe -> CC0-1.0
  10. github.com/streadway/handy -> [BSD 2-Clause ] from README.md
  11. gonum.org/v1/plot -> BSD-3-Clause

@ChrsMark ChrsMark added the Team:Integrations Label for the Integrations team label Sep 21, 2021
@ChrsMark ChrsMark self-assigned this Sep 21, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Sep 21, 2021
@mergify
Copy link
Contributor

mergify bot commented Sep 21, 2021

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 test_prom_upgrade upstream/test_prom_upgrade
git merge upstream/master
git push upstream test_prom_upgrade

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 21, 2021

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-09-23T11:12:32.328+0000

  • Duration: 21 min 34 sec

  • Commit: ba0da4d354a94bcb0d78582c49f0f6f8984fe2f0

Steps errors 5

Expand to view the steps failures

metricbeat-lint - make -C metricbeat check;make -C metricbeat update;make -C x-pack/metricbeat che
  • Took 2 min 1 sec . View more details here
  • Description: make -C metricbeat check;make -C metricbeat update;make -C x-pack/metricbeat check;make -C x-pack/metricbeat update;make check-no-changes;
List files to upload
  • Took 0 min 0 sec . View more details here
  • Description: ls -l src/github.com/elastic/beats/build/system-tests-*.tar.gz
x-pack/metricbeat-lint - make -C x-pack/metricbeat check;make -C x-pack/metricbeat update;make -C
  • Took 2 min 12 sec . View more details here
  • Description: make -C x-pack/metricbeat check;make -C x-pack/metricbeat update;make -C metricbeat check;make -C metricbeat update;make check-no-changes;
List files to upload
  • Took 0 min 0 sec . View more details here
  • Description: ls -l src/github.com/elastic/beats/build/system-tests-*.tar.gz
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: Error 'hudson.AbortException: script returned exit code 2'

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

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

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

It is weird that updating prometheus requires so many changes, including so many projects that need overrides in the license checker. But well, if this is needed, it LGTM, pinging some more people for awareness of these changes.

{"name": "github.com/lightstep/lightstep-tracer-common/golang/gogo", "licenceType": "MIT"}
{"name": "github.com/pascaldekloe/goe", "licenceType": "CC0-1.0"}
{"name": "github.com/streadway/handy", "licenceType": "BSD-2-Clause"}
{"name": "gonum.org/v1/plot", "licenceType": "BSD-3-Clause"}
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that the license checker cannot identify all these licenses 🤔

{"name": "github.com/lightstep/lightstep-tracer-common/golang/gogo", "licenceType": "MIT"}
{"name": "github.com/pascaldekloe/goe", "licenceType": "CC0-1.0"}
{"name": "github.com/streadway/handy", "licenceType": "BSD-2-Clause"}
{"name": "gonum.org/v1/plot", "licenceType": "BSD-3-Clause"}
Copy link
Member

Choose a reason for hiding this comment

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

Consider alphabetically sorting the whole file.

go.mod Outdated
cloud.google.com/go v0.83.0
cloud.google.com/go/bigquery v1.8.0
cloud.google.com/go/pubsub v1.3.1
cloud.google.com/go/storage v1.10.0
Copy link
Member

Choose a reason for hiding this comment

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

Pinging @endorama and @sayden for awareness on these changes.

go.mod Outdated
github.com/Azure/go-autorest/autorest/adal v0.9.15
github.com/Azure/go-autorest/autorest/azure/auth v0.4.2
github.com/Azure/go-autorest/autorest/date v0.3.0
github.com/Masterminds/semver v1.4.2
github.com/Microsoft/go-winio v0.4.15-0.20190919025122-fc70bd9a86b5
github.com/Microsoft/go-winio v0.4.16
Copy link
Member

Choose a reason for hiding this comment

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

Pinging @narph for awareness on these changes.

go.mod Outdated
@@ -28,7 +27,7 @@ require (
github.com/antlr/antlr4 v0.0.0-20200820155224-be881fa6b91d
github.com/apoydence/eachers v0.0.0-20181020210610-23942921fe77 // indirect
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5
github.com/aws/aws-lambda-go v1.6.0
github.com/aws/aws-lambda-go v1.13.3
Copy link
Member

Choose a reason for hiding this comment

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

Pinging @kaiyan-sheng and @aspacca for awareness on these changes.

go.mod Outdated
code.cloudfoundry.org/go-diodes v0.0.0-20190809170250-f77fb823c7ee // indirect
code.cloudfoundry.org/go-loggregator v7.4.0+incompatible
code.cloudfoundry.org/rfc5424 v0.0.0-20180905210152-236a6d29298a // indirect
github.com/Azure/azure-event-hubs-go/v3 v3.1.2
github.com/Azure/azure-sdk-for-go v37.1.0+incompatible
github.com/Azure/azure-sdk-for-go v55.2.0+incompatible
Copy link
Member Author

Choose a reason for hiding this comment

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

It's not that simple to upgrade this library since the interfaces have been changed and requires some tuning. Fortunately it will be upgraded properly by #28028 so we can wait for it.

@ChrsMark
Copy link
Member Author

Currently waiting for #28028 as described at #28023 (comment).

cc: @premendrasingh @vjsamuel

@ChrsMark
Copy link
Member Author

It is weird that updating prometheus requires so many changes, including so many projects that need overrides in the license checker. But well, if this is needed, it LGTM, pinging some more people for awareness of these changes.

It seems weird to me too :). It's also weird why the licence-checker could not identify the licences in so many libraries but it's most probably because in some projects the licence is not in the proper format. For example the licence of github.com/ajstarks/svgo (https://github.com/ajstarks/svgo/blob/master/LICENSE) is not in proper format and hence the tool fails to detect it. See for cross reference a valid one.

cc: @ruflin @masci

@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2021

This pull request does not have a backport label. Could you fix it @ChrsMark? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Sep 22, 2021
@ruflin
Copy link
Member

ruflin commented Sep 22, 2021

It really seems something is off here with the license part. Also for the NOTICE.txt file:

29,726 additions, 9,610 deletions not shown because the diff is too large

We should make sure to understand why this is.

@ChrsMark
Copy link
Member Author

@ruflin I just tried to update the Prometheus library and then the make notice command was failing due to several libs. So I was adding them one by one to make the command succeed. These libraries are dependencies of the prometheus library but because of reasons they cannot be verified by our checker. Reasons could be that we don't have CC-BY-4.0 and CC0-1.0 in our list or because our script cannot detect the license within the LICENCE file (maybe the LICENCE file is not format properly) so I had to manually define it in overrides.json.

@ChrsMark
Copy link
Member Author

Detailed licence checks for libs:

  1. github.com/ajstarks/svgo -> CC-BY-4.0
  2. github.com/bmizerany/pat -> from README.md
  3. github.com/cactus/go-statsd-client/statsd -> MIT
  4. github.com/dnaeon/go-vcr -> BSD-2-Clause
  5. github.com/golang/freetype -> GPL-2.0
  6. github.com/hudl/fargo -> [MIT] from README.md
  7. github.com/influxdata/line-protocol -> MIT
  8. github.com/lightstep/lightstep-tracer-common/golang/gogo -> MIT
  9. github.com/pascaldekloe/goe -> CC0-1.0
  10. github.com/streadway/handy -> [BSD 2-Clause ] from README.md
  11. gonum.org/v1/plot -> BSD-3-Clause

@ruflin
Copy link
Member

ruflin commented Sep 23, 2021

Thanks for the detailed investigation into the license part.

For the diff on the version that are updated: I have the suspicion many of these are not directly related to prometheus but got updated to the most recent version. We should update these libs but not as part of this PR. Ideally we would have update the libs already in the past.

What I suggest is that we do several smaller PR's. 1 for updating the cloud.google.com/*, one for github.com/Azure, one for aws etc.. If we do this and one of the libs has an issue, rolling back will be simple and easy to trace when the change happened. Ideally each "owner" of the projects that use these libs could open the PR itself. But likely it is faster if you quickly take care of it @ChrsMark and do it for them.

There is a chance that everything is connected and depends on each other and then we need to do it all at once, but I strongly hope this is not the case.

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2021

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 test_prom_upgrade upstream/test_prom_upgrade
git merge upstream/master
git push upstream test_prom_upgrade

@ChrsMark
Copy link
Member Author

ChrsMark commented Oct 4, 2021

@ruflin thanks for your feedback here! I agree with your suggestions to split the upgrades into smaller chunks.

github.com/Azure: we have already #28028 by @narph . ✅
github.com/Microsoft/go-winio @narph could you take care of it too?. PR: #28464
k8s: I will do the upgrade. PR: #28228
cloud.google.com/go : @sayden @endorama could take care of this upgrade?, PR: #28229
github.com/aws: @kaiyan-sheng @aspacca could you take this please?, PR: #28236

I can create a different meta issue for this if you prefer or we can keep using this issue as reference. I prefer keep using this one to avoid the extra overhead :).

@ruflin
Copy link
Member

ruflin commented Oct 11, 2021

@ChrsMark Thanks for putting this together. Lets keep it here. Seems like we already made quite a big of progress!

@ChrsMark
Copy link
Member Author

Continuing at #28716

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants