api docs and API simplifications [WIP] #112

beniwohli · 2017-12-04T15:09:34Z

This changes the API of exception tracking. It gets rid of the somewhat opaque data argument and instead uses explicit keyword arguments for context, custom etc.

It also removes Django-template-specific code in the exception tracking. It only worked when TEMPLATE_DEBUG was set to true (which shouldn't be the case in production code), and was never exposed in the UI.

simitt · 2017-12-08T13:46:13Z

docs/api.asciidoc

+
+[break]
+[[client-api-capture-exception]]
+==== `Client.capture_exception()`


This Client.capture_exception() looks like a class method, but then you actually call client.capture_exception()

I used a capital C in all the titles that of methods on Client, I thought it would be clear that it means "method on the Client class", didn't really think of the issue that it could be mistaken as a class method.

Class methods (or static methods) are used relatively seldom in Python, so I don't think most readers would be confused.

ok, maybe it is just me then, coming from a ruby background.

simitt · 2017-12-08T13:47:29Z

docs/api.asciidoc

@@ -0,0 +1,159 @@
+[[api]]
+== Public API


Why do you split it into Client, Error and Transaction API when you actually need a client instance for all of them?

How about calling it a Client API and then only use Error and Transaction later on?

This is one of the things I struggled with when structuring the docs. I though by putting the "Client API" first, the reader will know further down how to get a client.

One definitly should find his way around by the explanations you provide. I just think it is somehow confusing to describe it as three seperate APIs. I would only distinguish between the elasticapm and the elasticapm.Client API. And the split up elasticapm.Client into

getting an instance

error capturing

transaction capturing

simitt · 2017-12-08T13:54:11Z

docs/api.asciidoc

+ * `message`: The message as a string.
+ * `param_message`: Alternatively, a parametrized message as a dictionary.
+    The dictionary contains two values: `message`, and `params`.
+    This allows the APM server to group messages together that have share the same


This sentence is a bit malformed: This allows the APM server to group messages together that have share the same parametrized message.

simitt · 2017-12-08T13:56:01Z

docs/api.asciidoc

+===== Arguments
+
+ * `message`: The message as a string.
+ * `param_message`: Alternatively, a parametrized message as a dictionary.


What if one doesn't provide message nor param_message? Maybe a sentence like Either message or param_message need to be provided... Or is it ok to not provide any of them?

Excellent point, one of the two is needed

simitt · 2017-12-08T13:58:10Z

docs/api.asciidoc

+[[transaction-api]]
+=== Transaction API
+
+[break]


Although the usage is straightforward, it would be nice to also have an example in for the transaction cmds, as you also have examples for client and error related usage.

simitt · 2017-12-08T13:59:50Z

docs/api.asciidoc

+
+[break]
+[[api-elasticapm-instrument]]
+==== `elasticapm.instrument()`


This doesn't seem to be related to transaction specific cmds, so maybe you want to move it to its own section like Auto-Instrumentation.

simitt · 2017-12-08T14:01:43Z

docs/api.asciidoc

+===== Arguments
+
+ * `data`: A dictionary with the data to be attached. This should be a flat key/value `dict` object.
+ * `_key`: The key to use for this `dict` object. Defaults to `custom`.


Does the underscore in _key indicate it is private? The documented possibility of modifying the variable looks somehow weird.

simitt · 2017-12-08T14:02:58Z

docs/api.asciidoc

+ * `data`: A dictionary with the data to be attached. This should be a flat key/value `dict` object.
+ * `_key`: The key to use for this `dict` object. Defaults to `custom`.
+
+NOTE: This should only be overridden by framework integrations.


It would be great to add optional or mandatory to every parameter you describe. Sometimes you mention it is optional, sometimes you mention a default, sometimes none of both.

For begin_transaction and end_transaction it is not very clear to me if the params are required or not.

simitt · 2017-12-08T14:38:25Z

docs/api.asciidoc

+
+[break]
+[[api-set-transaction-name]]
+==== `elasticapm.set_transaction_name()`


Why is begin_transaction and end_transaction a method of client byt set_transaction_name and set_transaction_data are not?

in most places you'd use set_transaction_name & Co, you don't have a client instance or the current transaction. With this, we can put the difficulty of getting the current transaction into our code, instead of leaving it to the user to acquire the current transaction somehow.

At the moment, the current transaction is stored on a thread local. But that only works for singlethreaded code, so we probably need to change that in the future. The way it is now, that change would be nicely encapsulated.

How can you not have a current transaction when you want to override the name of the current transaction?
I ofc don't have the insights into the agent, but from a pure user perspective I'd somwhow expect that set_transaction_name and begin/stop_transaction is within the same context.
But this might be somehow out of scope for this PR anyways, just realized that when looking at the docs.

simitt

The documentation looks great @beniwohli!
I only left some minor comments.

simitt · 2017-12-11T11:17:12Z

docs/api.asciidoc

+
+Returns the id of the message as a string.
+
+NOTE: either the `message` or the `param_message` argument is required.


Could you start with a capital letter here?

simitt · 2017-12-11T11:19:09Z

docs/api.asciidoc

+== Public API
+
+The Elastic APM Python agent has several public APIs.
+Most of these APIs are not needed when using one of our supported frameworks (Django and Flask),


How about changing this to something like Most of the public API functionality is not needed when..

simitt · 2017-12-11T11:23:53Z

docs/api.asciidoc

+ * `data`: A dictionary with the data to be attached. This should be a flat key/value `dict` object.
+ * `_key`: The key to use for this `dict` object. Defaults to `custom`.
+
+NOTE: This should only be overridden by framework integrations.


For begin_transaction and end_transaction it is not very clear to me if the params are required or not.

simitt · 2017-12-11T11:24:38Z

docs/api.asciidoc

+This includes a wide range of standard library and 3rd party modules.
+A list of instrumented modules can be found in `elasticapm.intrumentation.register`.
+This function should be called as early as possibly in the startup of your application.
+For supported frameworks, this is called automatically. Example:


A link to the docs listing the supported framework would be a nice to have here.

* documents large parts of the public API * changed API of `capture_exception`, `capture_message`. Instead of an opaque `data` argument, they now have keyword arguments for `context`, `custom`, etc. * removed special handling of template errors in Django. This only ever worked with `TEMPLATE_DEBUG` enabled, and the data was never shown.

Forward-port from #112 * documents large parts of the public API * changed API of `capture_exception`, `capture_message`. Instead of an opaque `data` argument, they now have keyword arguments for `context`, `custom`, etc. * removed special handling of template errors in Django. This only ever worked with `TEMPLATE_DEBUG` enabled, and the data was never shown.

* documents large parts of the public API * changed API of `capture_exception`, `capture_message`. Instead of an opaque `data` argument, they now have keyword arguments for `context`, `custom`, etc. * removed special handling of template errors in Django. This only ever worked with `TEMPLATE_DEBUG` enabled, and the data was never shown. closes #112

beniwohli force-pushed the feature/api-docs-and-fixes branch 5 times, most recently from dab17f5 to cdbb607 Compare December 8, 2017 13:37

simitt reviewed Dec 8, 2017

View reviewed changes

beniwohli force-pushed the feature/api-docs-and-fixes branch 3 times, most recently from 7957f28 to 711543a Compare December 11, 2017 11:11

simitt reviewed Dec 11, 2017

View reviewed changes

simitt approved these changes Dec 11, 2017

View reviewed changes

beniwohli force-pushed the feature/api-docs-and-fixes branch from 711543a to 753865e Compare December 11, 2017 12:48

beniwohli force-pushed the feature/api-docs-and-fixes branch from 753865e to 0a94ad6 Compare December 11, 2017 13:30

beniwohli closed this in eb657ee Dec 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api docs and API simplifications [WIP] #112

api docs and API simplifications [WIP] #112

beniwohli commented Dec 4, 2017 •

edited

Loading

simitt Dec 8, 2017

beniwohli Dec 8, 2017

simitt Dec 8, 2017

simitt Dec 8, 2017

beniwohli Dec 8, 2017

simitt Dec 8, 2017

simitt Dec 8, 2017

simitt Dec 8, 2017

beniwohli Dec 8, 2017

simitt Dec 8, 2017

simitt Dec 8, 2017

simitt Dec 8, 2017

simitt Dec 8, 2017

simitt Dec 11, 2017

simitt Dec 8, 2017

beniwohli Dec 8, 2017

simitt Dec 8, 2017

simitt left a comment

simitt Dec 11, 2017

simitt Dec 11, 2017

simitt Dec 11, 2017

simitt Dec 11, 2017


		Returns the id of the message as a string.

		NOTE: either the `message` or the `param_message` argument is required.

api docs and API simplifications [WIP] #112

api docs and API simplifications [WIP] #112

Conversation

beniwohli commented Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beniwohli commented Dec 4, 2017 •

edited

Loading