Skip to content

Conversation

@tomlongridge
Copy link
Contributor

@tomlongridge tomlongridge commented Jun 5, 2019

adds version string (from platform.python_version()) to device.runtimeVersions.python payload

Goal

Augments reporting with the version of Python in use when an event occurred in case of issues in a specific Python version.

Design

Added data to device.runtimeVersions.python as per other notifiers.

Changeset

Changed

  • Added new field in configuration, set to platform.python_version() on init
  • notification and sessiontracker use this data in their payloads

Tests

  • Added unit tests for the presence of the data
  • Confirmed data is there in example project

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

@tomlongridge tomlongridge changed the title feat(notification): add Python runtime version information to device … feat(notification): add Python runtime version information to device data Jun 5, 2019
@tomlongridge tomlongridge requested a review from tobyhs June 5, 2019 14:34
@tomlongridge tomlongridge marked this pull request as ready for review June 5, 2019 16:25
Copy link

@tobyhs tobyhs left a comment

Choose a reason for hiding this comment

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

The code looks fine, but I haven't had too much time to check why the build is failing and if its related.

I pushed a branch to trigger a build and that passed, so there is a chance that it is related to the changes (but I'm not seeing it yet).

else:
self.hostname = None

self.runtime_versions = {"python": str(platform.python_version())}
Copy link

Choose a reason for hiding this comment

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

As far as I know, platform.python_version() returns a string; is there a particular reason you're using str here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I misread the docs when it said defaults to 0 for patch version.

TBH, it was an optimistic addition to try and resolve the build failures. I thought it was happening on Travis for py34 only, but the build history seems to suggest these failures are randomly happening but only on this branch.

They are all TypeError: string indices must be integers errors when examining the payload. However I cannot see how adding a new field to the dict should cause this. I might need to dig into the FilterDict code to see if this is somehow causing some unexpected changes to the structure.

Copy link

Choose a reason for hiding this comment

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

I can't tell if you were planning on removing the str call or not.

Copy link

@tobyhs tobyhs left a comment

Choose a reason for hiding this comment

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

lgtm as a re-build passed

…data

adds version string (from platform.python_version()) to device.runtimeVersions.python payload
@tomlongridge tomlongridge force-pushed the tom/add-runtime-version branch from ee7d5b1 to 6eabaf6 Compare June 14, 2019 14:40
@tomlongridge tomlongridge requested a review from tobyhs June 14, 2019 15:55
Copy link

@tobyhs tobyhs left a comment

Choose a reason for hiding this comment

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

lgtm

@tomlongridge tomlongridge merged commit 61937b9 into next Jun 18, 2019
@kattrali kattrali deleted the tom/add-runtime-version branch August 27, 2020 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants