[FEATURE] Add telemetry to request meta by default #18

eoghanmcilwaine · 2019-05-08T06:20:35Z

Checklist

setup.py

README.md

eoghanmcilwaine · 2019-05-13T03:04:08Z

Hey @shtrom would you be able to do another review on this when you get a chance? Cheers ;)

README.md

shtrom · 2019-05-13T05:28:43Z

release.sh

 	# update and commit local version file used by tracking telemetry
 	echo -e "\\nWriting version file..."
-	sed -i "s/^VERSION *=.*/VERSION = '${new_version}'/" ${VERSION_FILE}
+	sed -i "s/^__version__ *=.*/__version__ = '${new_version}'/" ${VERSION_FILE}


Ah, we also need to fix the Makefile now, to check the _version.py file, rather than the setup.py

https://github.com/Learnosity/learnosity-sdk-python/blob/master/Makefile#L50

Oh yeah, missed that one, thanks! Will update this line -

learnosity-sdk-python/Makefile

Line 50 in b53f296

dist-check-version: PKG_VER=v$(shell sed -n "s/^.*VERSION\s\+=\s\+'$[^']\+$'.*$$/\1/p" setup.py)

Yup, and this one so as to not confuse people

learnosity-sdk-python/Makefile

Line 54 in b53f296

$(error Version number $(PKG_VER) in setup.py does not match git tag $(GIT_TAG))

shtrom

Cleanup in the README and fix in the Makefile needed, but otherwise good to go.

README.md

shtrom · 2019-05-13T06:52:36Z

Makefile

 		$(PYTHON) setup.py bdist_wheel --universal
 dist-upload: dist-check-version clean test dist-upload-twine
-dist-check-version: PKG_VER=v$(shell sed -n "s/^.*VERSION\s\+=\s\+'\([^']\+\)'.*$$/\1/p" setup.py)
+dist-check-version: PKG_VER=v$(shell sed -n "s/^.*__version__\s\+=\s\+'\([^']\+\)'.*$$/\1/p" learnosity_sdk/_version.py)


You should be able to use \s to match any whitespace (space, tab, ...) (thought my \+ in the initial line was misguided, and * is indeed better).

Also, I'd rather not be too specific on the format of the version, we just want to extract anything that's between the two quotes, which is what my $[^']\+$ was doing.

@shtrom were you commenting on an earlier fixup where I had done this? (To make the sed regex run on macOS)

sed -n "s/^__version__[ ]*=[ ]*'$[0-9a-zA-Z\.]*$'$$/\1/p" learnosity_sdk/_version.py

I've since changed it back to something closer to the original regex. As it's probably more robust if it only works on linux.

Ah, yeah, I did, and was confused by a fixup, so I just copy/refresh/pasted my comment.

You can try to run a make dist-check-version, and make sure that the version is properly extracted from the version file by looking at the command it runs.

I'd like to 2nd using * instead of \+ here just to account for the event where a space is somehow accidentally removed.

LRN-20221

Simplify deployment instructions in README

README.md

flakeparadigm · 2019-05-15T00:27:28Z

ChangeLog.md

 - Add better context to `DataApiException`s
+- Telemetry support

 ### Security fixes:


This probably should have been mentioned in the previous PR, but would you mind fixing up this line to be ### Security instead, to match how we've done it in other places (and the examples)?

flakeparadigm · 2019-05-15T00:31:51Z

learnosity_sdk/request/init.py

+        """
+        We use telemetry to enable better support and feature planning. It is
+        however not advised to disable it, and it will not interfere with any usage.
+        """


I'd suggest moving this comment down to where we have the enable/disable methods as it's more likely someone will search for those than the private variable. (Just to increase visibility)

flakeparadigm · 2019-05-15T00:41:57Z

learnosity_sdk/request/init.py

                output['action'] = self.action

+        if self.__telemetry_enabled:
+            output['meta'] = self.get_meta()


We don't account for it in PHP, but in Node and Java I've made sure that we edit or add the meta object, instead of completely replacing it. I'm not sure if this is particularly important but might be useful to do here in case one of our APIs starts needing a meta field in any of its requests.

https://github.com/Learnosity/learnosity-sdk-nodejs/blob/master/index.js#L41-L51
https://github.com/Learnosity/learnosity-sdk-java/blob/master/src/main/java/learnositysdk/request/Telemetry.java#L56-L63

Seems a bit yucky to prejudge the actions of a future developer by checking for a condition that definitely isn't true at the moment, no? Output is created as an empty dict ~40 lines earlier and stuff is added to it as needed.

Actually, now that you point that out - the meta is supposed to be added to the actual request object. Not the root level object for the request.

So my point here is that a user could add a meta field to their request data (again, possible in the case that one of our many APIs starts using that field) and this would overwrite that data which the user is trying to send to our sever.

Ah, that makes sense, thanks @flakeparadigm.

flakeparadigm · 2019-05-15T00:56:13Z

setup.py


 setuptools.setup(
    author='Cera Davies',
    author_email='[email protected]',


We should probably update these Author details. Not sure what email we should use, though. Might need to double check with @sebablanco?

setuptools.setup( author='Learnosity', author_email='[email protected]', ... )

Lol yes, I noticed this and had made a note to find out what the email should be.

@alangarf has just created [email protected] for this.

Cheers 😁 👍

eoghanmcilwaine force-pushed the LRN-20221/feature/add-telemetry branch from 98bf98f to 3748f42 Compare May 8, 2019 06:27

shtrom reviewed May 9, 2019

View reviewed changes

setup.py Outdated Show resolved Hide resolved

shtrom reviewed May 9, 2019

View reviewed changes

README.md Show resolved Hide resolved

eoghanmcilwaine force-pushed the LRN-20221/feature/add-telemetry branch 4 times, most recently from f5cf571 to 07d4245 Compare May 13, 2019 00:40

shtrom reviewed May 13, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

shtrom reviewed May 13, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

eoghanmcilwaine force-pushed the LRN-20221/feature/add-telemetry branch from f6bf0c4 to 611382c Compare May 13, 2019 06:49

shtrom reviewed May 13, 2019

View reviewed changes

eoghanmcilwaine added 2 commits May 13, 2019 17:55

[FEATURE] Add telemetry to request meta by default

868533c

LRN-20221

[VENDOR] Security bumps for dependencies in setup.py

febeb9b

eoghanmcilwaine force-pushed the LRN-20221/feature/add-telemetry branch from 611382c to febeb9b Compare May 13, 2019 07:56

fixup! [FEATURE] Add telemetry to request meta by default

f5fbe54

Simplify deployment instructions in README

eoghanmcilwaine commented May 14, 2019

View reviewed changes

README.md Show resolved Hide resolved

eoghanmcilwaine merged commit 41a615f into master May 14, 2019

eoghanmcilwaine deleted the LRN-20221/feature/add-telemetry branch May 14, 2019 09:58

flakeparadigm reviewed May 15, 2019

View reviewed changes

[FEATURE] Add telemetry to request meta by default #18

[FEATURE] Add telemetry to request meta by default #18

Uh oh!

Conversation

eoghanmcilwaine commented May 8, 2019 • edited by shtrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

eoghanmcilwaine commented May 13, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shtrom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eoghanmcilwaine commented May 8, 2019 •

edited by shtrom

Loading