Cherrypy instrumentation #1410

TheAnshul756 · 2022-10-27T10:33:59Z

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #1130

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A

Does This PR Require a Core Repo Change?

No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

srikanthccv · 2022-10-29T07:58:45Z

...pentelemetry-instrumentation-cherrypy/src/opentelemetry/instrumentation/cherrypy/__init__.py

@@ -0,0 +1,140 @@
+from email.policy import default


srikanthccv · 2022-10-29T07:59:13Z

...pentelemetry-instrumentation-cherrypy/src/opentelemetry/instrumentation/cherrypy/__init__.py

+)
+from opentelemetry import trace, context
+from opentelemetry.semconv.trace import SpanAttributes
+from opentelemetry.instrumentation.falcon.version import __version__


srikanthccv · 2022-10-29T08:00:40Z

...pentelemetry-instrumentation-cherrypy/src/opentelemetry/instrumentation/cherrypy/__init__.py

+        return _instruments
+
+    def _instrument(self, **kwargs):
+        self._original_cherrypy_application = cherrypy._cptree.Application


There should be a better way to do this. We can't use the private symbols because they are not guaranteed to remain backward compatible.

srikanthccv · 2022-10-29T08:03:08Z

...pentelemetry-instrumentation-cherrypy/src/opentelemetry/instrumentation/cherrypy/__init__.py

+    def _instrument(self, **kwargs):
+        self._original_cherrypy_application = cherrypy._cptree.Application
+        cherrypy._cptree.Application = _InstrumentedCherryPyApplication
+        cherrypy.Application = _InstrumentedCherryPyApplication


https://docs.cherrypy.dev/en/latest/extend.html#per-request-functions

Issue with these hooks are that they needed to be called for every paths that we want to use them for. They do not apply directly to everywhere. source

srikanthccv · 2022-10-29T08:04:32Z

...opentelemetry-instrumentation-cherrypy/src/opentelemetry/instrumentation/cherrypy/package.py

@@ -0,0 +1,3 @@
+_instruments = ("cherrypy >= 1.0",)


Please also pin the upper version, there can be breaking changes b/w major version changes.

srikanthccv · 2022-10-29T08:05:31Z

...pentelemetry-instrumentation-cherrypy/src/opentelemetry/instrumentation/cherrypy/__init__.py

+                        span.set_attributes(custom_attributes)
+
+            if self.response_hook:
+                self.response_hook(span, status, response_headers)


Please add tests. If the PR is not ready then please mark it a draft.

Hey, I'm really sorry. I forgot to mark this PR draft. There's still lot of work required in this PR.

* added new testcases related to basic rest api calls and spans check for instrumentation * added tox changes and metrices testcases and fixed review comments * added metrices testcases and review comments fixed * added fixes for review comments Co-authored-by: rahmukhe <rahmukhe@appdynamics.com>

…ul756/opentelemetry-python-contrib into cherrypy-instrumentation

TheAnshul756 · 2022-12-03T15:42:22Z

I've came up with three different approaches for instrumentation in cherrypy and I wanted to discuss all three of them with there pros and cons.

Instrumentation with CherryPy Hooks

Unlike other frameworks, CherryPy does not have support for custom middleware. Instead, CherryPy provides some hook points where you can add your functions. Datadog uses this approach for instrumentation. But all the endpoints for which tools need to be used must be decorated by that tool.
Eg.

class Root(object):
    @cherrypy.expose
    @cherrypy.tools.logit()
    def index(self):
        return "hello world"

Because of it, this approach can only be used for manual instrumentation, not auto-instrumentation. Although there are some workarounds that I came up with (like monkey patching the expose function) but they were also not covering all the possible cases (what if the function is not exposed with expose decorator index.exposed = true).

Instrumentation with CherryPy Publisher Subscriber

CherryPy’s backbone consists of a bus system implementing a simple publish/subscribe messaging pattern. Simply put, in CherryPy everything is controlled via that bus.

Various channels are available in CherryPy, but only two were helpful to us, i.e. before_request and after_request. Scout Python APM Agent use this approach of instrumentation. But the challenge with this approach is the before_request channel triggers at a very early stage of the request handling cycle, even before the request object parses the environ dictionary from the HTTP message, because of which we can not do much in before_request (including span creation). Thus this approach leaves us with a significant limitation.

Instrumentation by Patching CherryPy Application class

This is how we have instrumented various frameworks, and this approach can also be done here. New Relics and Appdynamics agent also uses this approach. The issue with this approach is Application class is implemented in _cptree module in CherryPy and imported in __init__ module. We can patch the class imported in __init__, but this will not enable instrumentation as CherryPy creates the object of the Application class in _cptree and uses the Application from that module directly, which forces us to instrument Application class both in __init__ and _cptree. I have used this approach for instrumenting in my PR but @srikanthccv believes

We can't use the private symbols because they are not guaranteed to remain backwards compatible.

I've implemented all three approaches and tested them to understand their pros and cons. The first two approaches will also come with other challenges, such as handling exceptions. And the third approach, although being backward incompatible, is tried and tested and guarantees to work.
I need help knowing what to do next and which approach to proceed with. Also, if anyone has other ideas, I'm more than interested to know.

srikanthccv · 2022-12-05T17:11:17Z

This is how we have instrumented various frameworks, and this approach can also be done here.

I am not against the idea of patching the Application class but I don't think it is a good idea to patch the private/internals of the framework.

And the third approach, although being backward incompatible, is tried and tested and guarantees to work.

What is guaranteed here?

Overall, I don't think it's a good idea to patch the private/internals of the library. We have seen this fail with a few instrumentations, so they either had to be pinned to some minor version which works or rewritten to not patch private symbols and get it to work in general. I would like to hear others thoughts on this @open-telemetry/opentelemetry-python-contrib-approvers .

* added new testcases related to basic rest api calls and spans check for instrumentation * added tox changes and metrices testcases and fixed review comments * added metrices testcases and review comments fixed * added fixes for review comments * added custom header testcases for instrumentation * removed print statements * added return statement to cherrypy app Co-authored-by: rahmukhe <rahmukhe@appdynamics.com>

TheAnshul756 · 2022-12-06T09:16:33Z

What is guaranteed here?

Sorry for the misunderstanding. What I mean by 'guarantee' here is that with other approaches, some loopholes could be there with instrumentation, like some exceptions could be missed or the way the endpoints are exposed could affect the instrumentation. And this approach covers all the possible cases.

I am not against the idea of patching the Application class but I don't think it is a good idea to patch the private/internals of the framework.

I understand your concern, which is why I tried instrumenting with other approaches, but nothing seems to work completely unless someone has a better idea.

sanketmehta28 · 2022-12-19T12:32:43Z

instrumentation/opentelemetry-instrumentation-cherrypy/README.rst

@@ -0,0 +1,40 @@
+OpenTelemetry CherryPy Tracing


Please add an example how to use this instrumentation

sanketmehta28 · 2022-12-19T12:37:25Z

...pentelemetry-instrumentation-cherrypy/src/opentelemetry/instrumentation/cherrypy/__init__.py

+from typing import Collection
+
+from opentelemetry.util.http import parse_excluded_urls, get_excluded_urls, get_traced_request_attrs
+import cherrypy


organize the import statement properly

sanketmehta28 · 2022-12-19T12:38:21Z

...pentelemetry-instrumentation-cherrypy/src/opentelemetry/instrumentation/cherrypy/__init__.py

+
+
+class CherryPyInstrumentor(BaseInstrumentor):
+    """An instrumentor for FastAPI


Change the docstring from FastAPI to CherryPy. Also please add an example how to use it

sanketmehta28 · 2022-12-19T12:43:13Z

...pentelemetry-instrumentation-cherrypy/src/opentelemetry/instrumentation/cherrypy/__init__.py

+                activation.__exit__(None, None, None)
+            else:
+                activation.__exit__(
+                    type(exc),


you should use exception instead of exc just to make it more readable

sanketmehta28 · 2022-12-19T12:52:55Z

instrumentation/opentelemetry-instrumentation-cherrypy/tests/test_cherrypy.py

+        for key, _ in not_expected.items():
+            self.assertNotIn(key, server_span.attributes)
+
+class TestNonRecordingSpanWithCustomHeaders(TestBase, helper.CPWebCase):


why dont you inherit this class from the TestCherryPyBase and override all functions? This will remove some redundant code

sanketmehta28 · 2022-12-19T12:54:26Z

Also resolve the build checks

TheAnshul756 · 2022-12-20T07:11:19Z

Overall, I don't think it's a good idea to patch the private/internals of the library. We have seen this fail with a few instrumentations, so they either had to be pinned to some minor version which works or rewritten to not patch private symbols and get it to work in general. I would like to hear others thoughts on this.

@open-telemetry/opentelemetry-python-contrib-approvers Please share your thoughts on this.

TheAnshul756 and others added 4 commits October 10, 2022 15:52

add boilerplate for adding changes

444a29b

add instrumentor and patch __call__ method

bf6f816

Merge branch 'open-telemetry:main' into cherrypy-instrumentation

083b023

working instrumentation in cherrypy

215cd02

TheAnshul756 requested a review from a team October 27, 2022 10:34

srikanthccv reviewed Oct 29, 2022

View reviewed changes

TheAnshul756 marked this pull request as draft October 31, 2022 05:14

revert back to previous commit

4933b32

TheAnshul756 force-pushed the cherrypy-instrumentation branch from ea77939 to 4933b32 Compare November 28, 2022 09:04

TheAnshul756 and others added 6 commits November 28, 2022 16:32

Merge branch 'main' into cherrypy-instrumentation

0ac9dc1

fix lint

fa30e55

Merge branch 'cherrypy-instrumentation' of https://github.com/TheAnsh…

232c6ca

…ul756/opentelemetry-python-contrib into cherrypy-instrumentation

fix generate

2ed744e

Merge branch 'main' into cherrypy-instrumentation

c6db7ef

Merge branch 'main' into cherrypy-instrumentation

28f386c

rahulmukherjee68 and others added 3 commits December 6, 2022 14:20

Merge branch 'open-telemetry:main' into cherrypy-instrumentation

b37251f

fix get traced request attributes

41d94e9

sanketmehta28 requested changes Dec 19, 2022

View reviewed changes

dacevedo12 mentioned this pull request May 2, 2023

Instrumentation acceptance policy and general guidelines for external instrumentation authors #1714

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cherrypy instrumentation #1410

Cherrypy instrumentation #1410

TheAnshul756 commented Oct 27, 2022

srikanthccv Oct 29, 2022

srikanthccv Oct 29, 2022

srikanthccv Oct 29, 2022

srikanthccv Oct 29, 2022

TheAnshul756 Nov 3, 2022

srikanthccv Oct 29, 2022

srikanthccv Oct 29, 2022

TheAnshul756 Oct 31, 2022

TheAnshul756 commented Dec 3, 2022

srikanthccv commented Dec 5, 2022

TheAnshul756 commented Dec 6, 2022

sanketmehta28 Dec 19, 2022

sanketmehta28 Dec 19, 2022

sanketmehta28 Dec 19, 2022

sanketmehta28 Dec 19, 2022

sanketmehta28 Dec 19, 2022

sanketmehta28 commented Dec 19, 2022

TheAnshul756 commented Dec 20, 2022



		class CherryPyInstrumentor(BaseInstrumentor):
		"""An instrumentor for FastAPI

Cherrypy instrumentation #1410

Are you sure you want to change the base?

Cherrypy instrumentation #1410

Conversation

TheAnshul756 commented Oct 27, 2022

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

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

TheAnshul756 commented Dec 3, 2022

Instrumentation with CherryPy Hooks

Instrumentation with CherryPy Publisher Subscriber

Instrumentation by Patching CherryPy Application class

srikanthccv commented Dec 5, 2022

TheAnshul756 commented Dec 6, 2022

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

sanketmehta28 commented Dec 19, 2022

TheAnshul756 commented Dec 20, 2022