Skip to content

Add Collector name and version to Analytics API payload#13

Merged
KatieWright26 merged 1 commit into
mainfrom
pie-1353-add-collector-name-and-version-to-python
Jan 11, 2023
Merged

Add Collector name and version to Analytics API payload#13
KatieWright26 merged 1 commit into
mainfrom
pie-1353-add-collector-name-and-version-to-python

Conversation

@KatieWright26
Copy link
Copy Markdown
Contributor

@KatieWright26 KatieWright26 commented Jan 10, 2023

TA is working on displaying a tally of the types of collectors our customers are using. We can gather this data via the collector names and versions, however some collectors are missing this data (please see related linear ticket for list).

As I am unfamilar with python, I moved the NAME and VERSION values from the setup.py and defined them in a new file constants.py. This way we can share these values across file names while keeping them consistent. Happy to change if a Python speaker knows better!

This PR does not include the necessary version bump + publish to Python Package Index.

This collector will now emit the collector name "python-buildkite-test-collector"


Linear Ticket
Related Linear Ticket

@KatieWright26 KatieWright26 self-assigned this Jan 10, 2023
@KatieWright26 KatieWright26 force-pushed the pie-1353-add-collector-name-and-version-to-python branch 14 times, most recently from c646a04 to 4dcfad8 Compare January 10, 2023 01:48
@KatieWright26 KatieWright26 marked this pull request as ready for review January 10, 2023 01:50
from uuid import uuid4

import os
from .constants import COLLECTOR_NAME, VERSION # pylint: disable=W0611
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Python linter doesn't recognise that imports that are interpolated are actually used, so disabling for this line
https://pylint.pycqa.org/en/latest/user_guide/messages/warning/unused-import.html

"message": self.message,
"url": self.url
"url": self.url,
"collector": 'python-{COLLECTOR_NAME}',
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think I should be changing gem/package/library name in the setup.py, so I have instead added collector type clarity here by interpolating the language before the generic name (buildkite-test-collector is the same as the exlir and ruby collector names. The JS collector follows this pattern of interpolation the package name with the language).

@KatieWright26 KatieWright26 requested review from a team, jimsynz and swebb and removed request for jimsynz January 10, 2023 01:52
Copy link
Copy Markdown
Contributor

@gchan gchan left a comment

Choose a reason for hiding this comment

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

Code changes LGTM! I haven't tested or verified it works.

Can we update/add a test to verify the collector name and version is what we expect?

Maybe here (or another appropriate place)? https://github.com/buildkite/test-collector-python/blob/main/tests/buildkite_test_collector/collector/test_run_env.py#L32

@KatieWright26 KatieWright26 force-pushed the pie-1353-add-collector-name-and-version-to-python branch 2 times, most recently from cf57433 to 30d9932 Compare January 10, 2023 02:11
@KatieWright26 KatieWright26 force-pushed the pie-1353-add-collector-name-and-version-to-python branch from 30d9932 to 3ee08a7 Compare January 10, 2023 02:15
@KatieWright26
Copy link
Copy Markdown
Contributor Author

Code changes LGTM! I haven't tested or verified it works.

Can we update/add a test to verify the collector name and version is what we expect?

Maybe here (or another appropriate place)? https://github.com/buildkite/test-collector-python/blob/main/tests/buildkite_test_collector/collector/test_run_env.py#L32

Cool, have added - I really don't know how to test this as I also need to bump the package in another PR (I think). @swebb do you know?

@jimsynz
Copy link
Copy Markdown
Contributor

jimsynz commented Jan 10, 2023

Hi @KatieWright26. Good to see you! :)

@KatieWright26
Copy link
Copy Markdown
Contributor Author

Hi @KatieWright26. Good to see you! :)

Jimsy!! I saw your last name and couldn't remember if it was yours! Tiny NZ tech scene strikes again 😆

@KatieWright26 KatieWright26 added the enhancement New feature or request label Jan 11, 2023
@KatieWright26 KatieWright26 merged commit 60d6a21 into main Jan 11, 2023
@KatieWright26 KatieWright26 deleted the pie-1353-add-collector-name-and-version-to-python branch January 11, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants