New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding metric collection as part of instrumentations - Django #1230

Merged

lzchen merged 17 commits into open-telemetry:master from lzchen:django

Oct 16, 2020

Contributor

lzchen commented Oct 12, 2020 •

edited

Loading

Similar to #1116.

Adding the ability to collect duration metrics from Django instrumentation.

Semantic conventions for metrics can be found here

Currently django only supports SERVER type (since we don't have database tracking yet). The HTTPMetricRecorder is also being passed via settings from the DjangoInstrumentor to the middleware.

Also included in this PR:

a few changes to MetricMixin in the instrumentation package: Support both SERVER and CLIENT types, add a function to calculate duration without the use of context manager

lzchen added 4 commits

October 12, 2020 14:45


          django

89b4e6b


          tests

e869641


          Merge branch 'master' of https://github.com/open-telemetry/openteleme…

fb59669

…try-python into django


          changelog

585ca48

lzchen requested a review from a team

October 12, 2020 19:40

lzchen added 4 commits

October 12, 2020 15:59


          tests

ad5bf9a


          lint

1e71ed5


          lint

ccfba2f


          Update middleware.py

24cf8ff

owais reviewed

View reviewed changes

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py Show resolved Hide resolved

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py Show resolved Hide resolved

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py Outdated Show resolved Hide resolved

lzchen added 4 commits

October 13, 2020 10:54


          address comments

b2bf957


          fix test

6255c94


          Update middleware.py

720ed3a


          Update middleware.py

f25d2d8

owais approved these changes

View reviewed changes

Contributor

owais left a comment

LGTM

ocelotl suggested changes

View reviewed changes

Contributor

ocelotl left a comment

Please take a look at the comments, if you consider the current state of the code to be ok, let me know and I'll approve. 👍

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py Outdated Show resolved Hide resolved

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py Outdated

+                  def _get_metric_labels_from_attributes(attributes):
+                      labels = {}
+                      labels["http.method"] = attributes.get("http.method", "")
+                      for attrs in _ATTRIBUTES_BY_PREFERENCE:

Contributor

ocelotl Oct 13, 2020

Maybe just declare the _ATTRIBUTES_BY_PREFERENCE here since it is only used once?

Contributor Author

lzchen Oct 14, 2020

Defined once on module level so it doesn't have to be defined everytime a request is called.

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py Outdated Show resolved Hide resolved

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py Outdated Show resolved Hide resolved

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

+                          if all(labels_from_attributes.values()):
+                              labels.update(labels_from_attributes)
+                              break
+                      if attributes.get("http.flavor"):

Contributor

ocelotl Oct 13, 2020

This may have the same problem.

Suggested change

      
                    if attributes.get("http.flavor"):
          
                    if attributes.get("http.flavor") is not None:

Contributor Author

lzchen Oct 14, 2020

I don't believe we want to set the label if value is an empty string.

Contributor

ocelotl Oct 15, 2020 •

edited

Loading

Nevertheless, we may end up setting the label when the value is an empty string here?

Contributor Author

lzchen Oct 16, 2020

I think http.method should always be populated. That was just for sanity.

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py Outdated Show resolved Hide resolved


          address comments

4790aec

lzchen requested a review from ocelotl

October 14, 2020 17:46

lzchen added 2 commits

October 14, 2020 13:47


          Merge branch 'master' of https://github.com/open-telemetry/openteleme…

8bd1a61

…try-python into django


          Merge branch 'master' into django

1111bc6

lzchen assigned ocelotl

ocelotl approved these changes

View reviewed changes

Contributor

ocelotl left a comment

Approved since most of my comments have been marked as resolved, only leaving a question for @lzchen just in case.

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

+                          if all(labels_from_attributes.values()):
+                              labels.update(labels_from_attributes)
+                              break
+                      if attributes.get("http.flavor"):

Contributor

ocelotl Oct 15, 2020 •

edited

Loading

Nevertheless, we may end up setting the label when the value is an empty string here?

lzchen added 2 commits

October 15, 2020 20:06


          Merge branch 'master' of https://github.com/open-telemetry/openteleme…

a68ab76

…try-python into django


          Merge branch 'django' of https://github.com/lzchen/opentelemetry-python…

… into django

lzchen merged commit beccc3b into open-telemetry:master

lzchen deleted the django branch

October 16, 2020 00:24

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request


          chore(deps): update commitlint monorepo to v9 (major) (open-telemetry…

21fae0a

…#1230)

Co-authored-by: Mayur Kale <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet