Add Vertica Integration #634

Kyle-Verhoog · 2018-10-08T19:58:14Z

Overview

This PR adds support for the vertica-python library.

The traced methods include:

vertica_python.vertica.cursor.Cursor

execute()
copy()
fetchone()
fetchall()
nextset()

along with relevant metadata for each method.

Examples/Usage

Usage and examples can be found in the traces examples repo here: https://github.com/DataDog/trace-examples.

Development Note

This PR also introduces a proof of concept of a new instrumentation mechanism that aims to make writing the majority of integrations easy, configurable and consistent. For instance, this is the only code needed to instrument the execute method in the Vertica library:

def execute_span_start(instance, span, conf, *args, **kwargs):
    span.set_tag("query", args[0])

def execute_span_end(result, instance, span, conf, *args, **kwargs):
    span.set_metric(dbx.ROWCOUNT, instance.rowcount)

# tracing configuration
config._add(
    "vertica",
    {
        "service_name": "vertica",
        "app": "vertica",
        "app_type": "db",
        "patch": {
            "vertica_python.vertica.cursor.Cursor": {
                "routines": {
                    "execute": {
                        "operation_name": "vertica.query",
                        "span_type": "vertica",
                        "span_start": execute_span_start,
                        "span_end": execute_span_end,
                    },
                },
            },
        },
    },
)

For now this API is made private and contained within the Vertica integration. It is to be tested solely with the Vertica integration.

This API is subject to change and should not be used outside the Vertica integration.

Kyle-Verhoog · 2018-10-08T20:13:00Z

ddtrace/contrib/vertica/patch.py

+                    "execute": {
+                        "operation_name": "vertica.query",
+                        "span_type": "vertica",
+                        "on_before": execute_before,


I don't like this naming, but I'm unsure of what to call these. Maybe pre_trace, post_trace?

trace_start trace_end?

span_start span_end/span_finish, would be inline with span_type, and operation_name.

s/operation_name/resource_name/?

ddtrace/contrib/vertica/patch.py

Kyle-Verhoog · 2018-10-08T20:15:40Z

ddtrace/contrib/vertica/patch.py

+                        _config=config["patch"][_patch_item],
+                    ).onto(instance)
+
+                def _find_config(instance, routine_name):


This is temporary/a placeholder until a better solution is thought up. This code is not run in the vertica integration since the situation in which it is needed (described in the long comment below) does not occur in the tracing configuration for the vertica library.

this can probably be defined at module level rather than in the for loop ?

brettlangdon

Just did a first pass, a lot of comments here are mostly focused on style.

I want to give it a review through again focusing more on the overall strategy here.

This is a big one to review.

brettlangdon · 2018-10-08T22:23:08Z

ddtrace/contrib/vertica/constants.py

@@ -0,0 +1,4 @@
+


so many spaces

ddtrace/contrib/vertica/patch.py

brettlangdon · 2018-10-08T22:35:11Z

ddtrace/contrib/vertica/patch.py

+
+def _uninstall(config):
+    for patch_class_path in config["patch"]:
+        patch_mod = '.'.join(patch_class_path.split('.')[0:-1])


patch_mod, _, patch_class = patch_class_path.rpartition('.')

brettlangdon · 2018-10-08T22:36:04Z

ddtrace/contrib/vertica/patch.py

+
+def _install(config):
+    for patch_class_path in config["patch"]:
+        patch_mod = '.'.join(patch_class_path.split('.')[0:-1])


patch_mod, _, patch_class = patch_class_path.rpartition('.')

brettlangdon · 2018-10-08T22:39:08Z

ddtrace/contrib/vertica/patch.py

+
+                # patch the __init__ of the class with a Pin instance containing the defaults
+                @wrapt.patch_function_wrapper(patch_mod, "{}.{}".format(patch_class, "__init__"))
+                def init_wrapper(wrapped, instance, args, kwargs):


Should this patching live outside of the loop on L133?

Seems like this only needs to be called once per class, not once per-routine?

ddtrace/contrib/vertica/patch.py

brettlangdon · 2018-10-08T22:48:29Z

ddtrace/contrib/vertica/patch.py

+        for patch_routine in config["patch"][patch_class_path]["routines"]:
+            def _wrap():
+                # _wrap is needed to provide data to the wrappers which can
+                # only be provided from a function closure.


Not sure I follow why this is needed?

This is a big function, very easily I could have read over the part that makes this necessary.

brettlangdon · 2018-10-08T22:50:57Z

ddtrace/ext/db.py

@@ -2,3 +2,4 @@
 # tags
 NAME = "db.name"     # the database name (eg: dbname for pgsql)
 USER = "db.user"     # the user connecting to the db
+ROWCOUNT = "db.rowcount" # the rowcount of a query


should be two spaces after the line before #

also, might want to add spaces to the lines above to keep them inline (assuming that was the intention there)

brettlangdon · 2018-10-08T22:51:16Z

ddtrace/monkey.py

@@ -42,6 +42,7 @@
    'aiopg': True,
    'aiobotocore': False,
    'httplib': False,
+    "vertica": True,


brettlangdon · 2018-10-08T22:53:03Z

ddtrace/contrib/vertica/patch.py

+def _uninstall(config):
+    for patch_class_path in config["patch"]:
+        patch_mod = '.'.join(patch_class_path.split('.')[0:-1])
+        patch_class = patch_class_path.split('.')[-1]


s/'/"/g

It seems like the codebase isn't the most consistent with quotes right now, but since the rest of this file uses " we should do so here, and everywhere else in this file, as well (but I think I get why you did double quotes everywhere, but single quotes for single character strings)

.circleci/config.yml

labbati · 2018-10-09T07:17:07Z

ddtrace/contrib/vertica/patch.py

+        "app_type": "db",
+        "patch": {
+            "vertica_python.vertica.connection.Connection": {
+                "routines": {


We already discussed this in PM, an alternative approach could be to have:

"vertica_python.vertica.connection.Connection": { "cursor": { ... } "__other_metadata": ... }

The pattern that I mean is not adding the routines level now, and use a keyword __xxxx later on if we need to configure at the Connection level. Not saying it is required, just triggering a conversation about it.

labbati · 2018-10-09T07:28:56Z

ddtrace/contrib/vertica/patch.py

+
+_PATCHED = False
+
+def copy_before(instance, span, conf, *args, **kwargs):


Just thinking out loud. What if we replace def copy_before(): and def execute_before() with a function def tag_query() and then in the configuration:

... "execute": { "on_before": tag_query }, "copy": { "on_before": tag_query } ...

In the future (now seems to be not necessary), "on_before" etc... could also be passed a list of operations, instead of only one.

This adds a few benefits IMO:

greater direction between the function name and what it does

we do not lose the relation between the "on_before" and the fact that a tag is set

less code repetition

This also apply to execute_after and fetch_after.

labbati · 2018-10-09T07:30:25Z

ddtrace/contrib/vertica/patch.py

+
+
+# tracing configuration
+config._add(


@Kyle-Verhoog did I already mention that I love the idea? 😄 I think so 😄

brettlangdon · 2018-10-09T11:51:22Z

ddtrace/contrib/vertica/patch.py

+
+        def wrap_init():
+            _patch_item = patch_class_path
+            patch_class_routine = "{}.{}".format(patch_class, "__init__")


What if we need to patch a module function rather than a class method?

@Kyle-Verhoog (idk if that works) note to self: we still need to address this later on.

tox.ini

ddtrace/contrib/vertica/patch.py

brettlangdon · 2018-10-10T11:26:48Z

ddtrace/contrib/vertica/patch.py

+        patch_mod, _, patch_class = patch_class_path.rpartition(".")
+
+        for patch_routine in config["patch"][patch_class_path]["routines"]:
+            mod = importlib.import_module(patch_mod)


This should be moved outside of the loop.

ddtrace/contrib/vertica/patch.py

brettlangdon · 2018-10-10T11:28:50Z

ddtrace/contrib/vertica/patch.py

+
+        config_routines = config["patch"][full_name]["routines"]
+
+        if full_name in config["patch"] and routine_name in config_routines:


s/full_name in config["patch"] and //

We are already doing that assertion above.

brettlangdon · 2018-10-10T11:29:57Z

ddtrace/contrib/vertica/patch.py

+                    _config=config["patch"][_patch_item],
+                ).onto(instance)
+
+        wrap_init()


Why does this need to be a function, can we not just execute the code immediately?

The reasoning behind writing these as inline functions is that it allowed me to quickly change around the logic without having to constantly update the signatures. That said, if we're happier with this approach we can clean these up into helper functions 😄

brettlangdon · 2018-10-10T11:40:54Z

tests/contrib/vertica/test_vertica.py

+from .utils import override_config
+
+
+TEST_TABLE = "test_table"


could just import from .fixtures import TEST_TABLE

tests/contrib/vertica/test_vertica.py

tox.ini

brettlangdon

Some other small comments, but mostly looking good.

brettlangdon · 2018-10-10T14:47:44Z

ddtrace/contrib/vertica/patch.py

+    span.set_metric(dbx.ROWCOUNT, instance.rowcount)
+
+
+def cursor_after(instance, cursor, span, conf, *args, **kwargs):


Should this be cursor_span_end to match the others?

brettlangdon · 2018-10-10T14:50:03Z

ddtrace/contrib/vertica/patch.py

+            "vertica_python.vertica.connection.Connection": {
+                "routines": {
+                    "cursor": {"trace_enabled": False, "span_end": cursor_after}
+                }


trailing comma here and line above.

brettlangdon · 2018-10-10T15:01:16Z

ddtrace/contrib/vertica/patch.py

+        # TODO?: remove Pin dependence
+        pin = Pin.get_from(instance)
+
+        # TODO: inheritance problem with pins:


You can likely remove this TODO, but keep some form of this comment so we know the problem/what we are doing.

ddtrace/contrib/vertica/patch.py

labbati · 2018-10-11T13:52:54Z

ddtrace/contrib/vertica/patch.py

+
+
+# tracing configuration
+config._add(


I am currently working on the settings module and one thing came to my mind. How this approach fit into the user workflow. I mean if a user provides some settings through the config api would we override them completely?

We will have to figure out how exactly we want to expose the config api to the user for them to modify. This is something that is still to be determined. In the documentation for Vertica I have only mentioned a custom service_name. This is all that I think we want to expose for now.

I'll look into adding more traditional customization via the Pin api to ensure we maintain that compatibility.

Good catch!

ddtrace/contrib/vertica/patch.py

labbati

I erroneously instead of commenting, please see at my only comment I added

brettlangdon · 2018-10-11T14:17:09Z

ddtrace/contrib/vertica/patch.py

+    span.set_metric(dbx.ROWCOUNT, instance.rowcount)
+
+
+def cursor_span_end(instance, cursor, _, conf, *args, **kwargs):


brettlangdon

lgtm

labbati · 2018-10-11T15:28:03Z

ddtrace/contrib/vertica/patch.py

+
+
+# tracing configuration
+config._add(


Kyle-Verhoog added 14 commits October 8, 2018 21:02

[verticadb] add skeleton code and test infra

7893b7a

[vertica] add experimental tracing api

ba5c45b

[vertica] move pin config to connection

4f24146

[vertica] add fixtures and exception test

46629d2

[vertica] switch approach to proxy methods only once

5a61be4

[vertica] switch approach to patch once on module import

36a883b

[vertica] remove duplicated lines

c5d257b

[vertica] experiment with using Pin for api to configuration

c058a99

[vertica] add config find logic and more tests

0c3dd4f

[vertica] add circleci job

54ff530

[vertica] rename verticadb -> vertica

cdd455a

[vertica] fix circleci typo

4b408f2

[vertica] clean up, add a test for copy()

3cc04a1

[vertica] add basic documentation

ea66e9d

Kyle-Verhoog added the integrations label Oct 8, 2018

Kyle-Verhoog added this to the 0.15.0 milestone Oct 8, 2018

Kyle-Verhoog self-assigned this Oct 8, 2018

Kyle-Verhoog added 2 commits October 8, 2018 22:06

[vertica] add user and database metadata

73bde5f

[vertica] format tests

53c3976

Kyle-Verhoog commented Oct 8, 2018

View reviewed changes

ddtrace/contrib/vertica/patch.py Show resolved Hide resolved

Kyle-Verhoog commented Oct 8, 2018

View reviewed changes

[vertica] use default docker image database name

55cd8c5

Kyle-Verhoog changed the base branch from master to 0.15-dev October 8, 2018 20:44

Merge branch '0.15-dev' into vertica

571fff8

brettlangdon reviewed Oct 8, 2018

View reviewed changes

labbati reviewed Oct 9, 2018

View reviewed changes

Kyle-Verhoog added 3 commits October 9, 2018 11:46

[vertica] code clean-up and formatting

fbe4797

[vertica] add wait logic for container start up

993c03d

[vertica] use __mro__ instead of hacky __bases__ looping

528d6f0

brettlangdon reviewed Oct 9, 2018

View reviewed changes

Kyle-Verhoog commented Oct 9, 2018

View reviewed changes

tox.ini Show resolved Hide resolved

Kyle-Verhoog and others added 5 commits October 9, 2018 15:08

[vertica] clean-up

377163d

[vertica] move _find_config to module level, rename

f717941

[vertica] add vertica dependency for wait services

79c73ea

[vertica] formatting

88c72cb

Merge branch '0.15-dev' into vertica

162c397

Kyle-Verhoog requested a review from palazzem October 9, 2018 14:08

Kyle-Verhoog added 2 commits October 9, 2018 18:20

[vertica] use proper span_type so queries are quantized

858220f

[vertica] update span_type tests

bdf4af7

Kyle-Verhoog commented Oct 9, 2018

View reviewed changes

ddtrace/contrib/vertica/patch.py Show resolved Hide resolved

brettlangdon reviewed Oct 10, 2018

View reviewed changes

Kyle-Verhoog added 4 commits October 10, 2018 15:40

[vertica] clean up code

ae44fa9

[vertica] fix mistakenly passing test

e49b7ae

[vertica] set resource as query so it can be quantized by the agent

c81e760

[vertica] add check for existence of class when unpatching

1ef4ee0

brettlangdon reviewed Oct 10, 2018

View reviewed changes

Kyle-Verhoog added 2 commits October 10, 2018 17:28

[vertica] clean up dev notes

6121893

[vertica] tests cleanup

5d50106

labbati approved these changes Oct 11, 2018

View reviewed changes

labbati reviewed Oct 11, 2018

View reviewed changes

brettlangdon reviewed Oct 11, 2018

View reviewed changes

brettlangdon approved these changes Oct 11, 2018

View reviewed changes

palazzem approved these changes Oct 11, 2018

View reviewed changes

Kyle-Verhoog added 2 commits October 11, 2018 17:24

[vertica] verify that a custom service name can be set via Pin

68aeb0f

[vertica] update the docs

7df408c

labbati approved these changes Oct 11, 2018

View reviewed changes

ddtrace/contrib/vertica/patch.py

# tracing configuration

config._add(

Copy link

Member

labbati Oct 11, 2018

Choose a reason for hiding this comment

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

👍

brettlangdon changed the base branch from 0.15-dev to master October 11, 2018 16:00

Kyle-Verhoog merged commit 6c1bbf6 into master Oct 11, 2018

Kyle-Verhoog deleted the vertica branch October 11, 2018 16:04


		_PATCHED = False

		def copy_before(instance, span, conf, args, *kwargs):


		config_routines = config["patch"][full_name]["routines"]

		if full_name in config["patch"] and routine_name in config_routines:

		span.set_metric(dbx.ROWCOUNT, instance.rowcount)


		def cursor_after(instance, cursor, span, conf, args, *kwargs):

		span.set_metric(dbx.ROWCOUNT, instance.rowcount)


		def cursor_span_end(instance, cursor, _, conf, args, *kwargs):

Add Vertica Integration #634

Add Vertica Integration #634

Conversation

Kyle-Verhoog commented Oct 8, 2018 • edited

Overview

Examples/Usage

Development Note

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

brettlangdon 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

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

brettlangdon 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

labbati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettlangdon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kyle-Verhoog commented Oct 8, 2018 •

edited