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

Remove the underlying implementation of anonymous-telemetry #18952

Merged
merged 8 commits into from
May 10, 2023

Conversation

thejcannon
Copy link
Member

@thejcannon thejcannon commented May 9, 2023

This is the reason requests (and therefore other dependencies) is a dependency of Pants. This yields a smaller Pants footprint. Lastly, as Carina says in Slack:

We originally expected it would help us provide better tech support, but it hasn't really panned out for that. If we could substitute in some other telemetry tool, that could be worthwhile. But given that no major value being is being returned to the project and it has unfortunately become a blocker for some users, I think it's time to take humbug out of people's way one way or another.

After:

$ pants package src/python/pants:pants-packaged
$ python -m venv .venv
$ .venv/bin/pip install dist/pantsbuild.pants-2.17.0.dev5-cp38-cp38-linux_x86_64.whl
...
Successfully installed PyYAML-6.0 ansicolors-1.1.8 chevron-0.14.0 fasteners-0.16.3 ijson-3.1.4 importlib-resources-5.0.7 node-semver-0.9.0 packaging-21.3 pantsbuild.pants-2.17.0.dev5 pex-2.1.135 psutil-5.9.0 pyparsing-3.0.9 python-lsp-jsonrpc-1.0.0 setproctitle-1.3.2 setuptools-63.4.3 six-1.16.0 toml-0.10.2 types-PyYAML-6.0.3 types-setuptools-62.6.1 types-toml-0.10.8 typing-extensions-4.3.0 ujson-5.7.0
$ .venv/bin/python
Python 3.8.10 (default, Jun 22 2022, 20:18:18) 
[GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'requests'
>>> quit()

We're keeping the subsystem in place so that we could swap in another implementation at a later date.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Yay simplicity

src/python/pants/goal/anonymous_telemetry.py Outdated Show resolved Hide resolved
src/python/pants/goal/anonymous_telemetry.py Outdated Show resolved Hide resolved
Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

I'm generally in favor, but has there been discussion of removing this? Seems like something Maintainers should agree on

@thejcannon
Copy link
Member Author

We've mentioned it informally, but also join in: https://pantsbuild.slack.com/archives/C0D7TNJHL/p1683644499629429

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

I'm generally in favor, but has there been discussion of removing this? Seems like something Maintainers should agree on

Agree it would be a good thing to have maintainers agree to this before landing, but see no issue to why we couldn't get that here? (and if it becomes a hot topic, may of course want to redirect longer discussions elsewhere)

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

I am in favor of removing humbug as the implementation of anonymous telemetry, but I do wonder whether there isn't some other very simple web api we could use to report this same data, which might actually result in it getting consumed more often than the humbug data did?

One option that might reduce churn would be to leave all of the actual public infrastructure of anonymous telemetry in place (the subsytem, docs, etc), but to remove the recording backend. We would then schedule the deprecation of the subsystem for a future version, to leave room for another implementation (and the motivation to build it) to present itself.

3rdparty/python/requirements.txt Show resolved Hide resolved
@thejcannon
Copy link
Member Author

Hmm I've thought of that as well @stuhood, so I'm in favor. Let me switch this to that.

@thejcannon thejcannon changed the title Deprecate anonymous-telemetry and remove humbug Remove the underlying implementation of anonymous-telemetry May 9, 2023
@thejcannon
Copy link
Member Author

Should we also remove src/python/pants/goal/run_tracker.py's get_anonymous_telemetry_data? It'd be dead code.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

We're not using it right now. I'm fine with removing the humbug-based implementation. Once we have something better, we can re-introduce it.

If a user has telemetry enabled and is using an older branch, and if we do not renew our telemetry storage solution, will those users have any issues? Do we need to cherry-pick this to old branches before we can fully sunset the telemetry service?

@thejcannon
Copy link
Member Author

If a user has telemetry enabled and is using an older branch, and if we do not renew our telemetry storage solution, will those users have any issues?

No, part of the guarantees is all forms of failure are silent.

@huonw
Copy link
Contributor

huonw commented May 9, 2023

Since the consensus seems to be not cherry-picking this, I proposed #18959 as a focused fix for cherry-picking, and this one can then remove that workaround too.

After:

$ pants paths --from=src/python/pants:pants-packaged --to=3rdparty/python#requests
[]

Minor, but due to #12733 (or similar), this gives the same result before this change: [].

So, we might want too confirm the change another way, e.g. (not sure if there's easier ways):

  • check the wheel's METADATA file (and all of the dependencies of everything listed there...)
  • install the wheel into a fresh venv and see what else is installed

@thejcannon
Copy link
Member Author

Oh duh lol

@sureshjoshi
Copy link
Member

I will always and forever +1 telemetry removal (even anonymous telemetry).

I would also be a +1 for cherry picking this - and deprecating the current anonymous_telemetry field.

I mean, it's "technically" a functionality change, but in practice - it's not end-user or development-user-centric functionality.

But I'll defer to Pants convention on the cherry pick.

@thejcannon
Copy link
Member Author

Going to merge. Thanks for participating y'all.

@thejcannon
Copy link
Member Author

(updated PR with manual testing steps)

@thejcannon thejcannon merged commit eb0e836 into pantsbuild:main May 10, 2023
@thejcannon thejcannon deleted the byebyehumbug branch May 10, 2023 23:59
huonw added a commit that referenced this pull request May 11, 2023
…) (#18971)

The release of urllib3 2 has caused issues with installing pants (e.g.
it requires OpenSSL 1.1.1), so, for now, we can restrict to only working
with urllib3 1.x.y and thus reduce how often people have to apply
workarounds like `PIP_CONSTRAINTS=...`.

I've verified that the wheel `METADATA` has `Requires-Dist: urllib3
(<2)`, and installing the wheel into a fresh venv _before_ uses
`urllib3==2.0.2`, while installing the wheel _after_ uses
`urllib3==1.26.15`.

This patch is intended to be an option for a short term/focused
work-around that is safe to cherry-pick back to older branches, while
#18952 is the better fix (removes the use of urllib3 from the main
wheel) but riskier, and thus might not be cherry-picked.

Background:
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1683644499629429
huonw added a commit that referenced this pull request May 11, 2023
…) (#18973)

The release of urllib3 2 has caused issues with installing pants (e.g.
it requires OpenSSL 1.1.1), so, for now, we can restrict to only working
with urllib3 1.x.y and thus reduce how often people have to apply
workarounds like `PIP_CONSTRAINTS=...`.

I've verified that the wheel `METADATA` has `Requires-Dist: urllib3
(<2)`, and installing the wheel into a fresh venv _before_ uses
`urllib3==2.0.2`, while installing the wheel _after_ uses
`urllib3==1.26.15`.

This patch is intended to be an option for a short term/focused
work-around that is safe to cherry-pick back to older branches, while
#18952 is the better fix (removes the use of urllib3 from the main
wheel) but riskier, and thus might not be cherry-picked.

Background:
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1683644499629429
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants