Skip to content
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.

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

Adds user agent string to grpc headers #3009

Merged
merged 8 commits into from
Dec 6, 2022

Conversation

pridhi-arora
Copy link
Contributor

@pridhi-arora pridhi-arora commented Nov 1, 2022

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 #2958

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 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 Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR:

  • No.

Checklist:

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

@pridhi-arora pridhi-arora requested a review from a team as a code owner November 1, 2022 11:52
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 1, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. You also need to add unit tests.

CHANGELOG.md Outdated Show resolved Hide resolved
@pridhi-arora
Copy link
Contributor Author

@srikanthccv Thanks for the review; I am working on tests.

@pridhi-arora
Copy link
Contributor Author

Hi @srikanthccv, Can you give me a small cue as to how I add a test and run it? Should I add a line below this and something like this? Thanks in advance.

@srikanthccv
Copy link
Member

You could add an another assert statement in this test which checks that user-agent header is there. You need to do this for all three exporters.

@pridhi-arora
Copy link
Contributor Author

@srikanthccv Please take a look.

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Please resolve the merge conflicts and address the comments

@pridhi-arora
Copy link
Contributor Author

@srikanthccv Please take a look.

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

It's in better shape than last time. Few more comments and we should be able to get this merged after them.

@pridhi-arora
Copy link
Contributor Author

pridhi-arora commented Nov 14, 2022

@srikanthccv I am stuck at log exporter tests. Could you help me proceed?
This is max I could make sense of
https://github.com/open-telemetry/opentelemetry-python/pull/3009/files#diff-b46561f17e94b2de768ab7d81658ef144122a638a7ffb3fcd82fe122a9561b37R256-R268

@srikanthccv
Copy link
Member

Are the tests passing locally? I don't think the OTLP log exporter supports OTEL_EXPORTER_OTLP_LOG_HEADERS. Please look at what the metric/trace exporter is doing and do the same for the log exporter. Let me know if you need more clarity or an example of what the test would look like.

@pridhi-arora
Copy link
Contributor Author

The tests are not passing locally. I saw that OTEL_EXPORTER_OTLP_METRICS_HEADERS and OTEL_EXPORTER_OTLP_METRICS_HEADERS are part ofopentelemetry.sdk.environment_variables SO, I added a similar field called as OTEL_EXPORTER_OTLP_LOG_HEADERS. The tests fail as

AssertionError: Tuples differ: (('user-agent', 'OTel OTLP Exporter Python/1.15.0.dev'),) != (('key1', 'value1'), ('key2', 'VALUE=2'), ('[48 chars]ev'))
E
E       First differing element 0:
E       ('user-agent', 'OTel OTLP Exporter Python/1.15.0.dev')
E       ('key1', 'value1')
E
E       Second tuple contains 2 additional elements.
E       First extra element 1:
E       ('key2', 'VALUE=2')
E
E       + (('key1', 'value1'),
E       +  ('key2', 'VALUE=2'),
E       - (('user-agent', 'OTel OTLP Exporter Python/1.15.0.dev'),)
E       ? ^                                                      -
E
E       +  ('user-agent', 'OTel OTLP Exporter Python/1.15.0.dev'))
E       ? ^

logs/test_otlp_logs_exporter.py:261: AssertionError

Logger doesn't call OTLPExporterMixin.init.

If you think I need to call this function in the logger. Shouldn't that be a separate PR?

@pridhi-arora
Copy link
Contributor Author

@srikanthccv I found an issue you filed a while ago which relates to what you are requesting.
#2939

@srikanthccv
Copy link
Member

You can ignore the OTEL_EXPORTER_OTLP_LOG_HEADERS and add an assertion that just a user-agent exists.

@pridhi-arora
Copy link
Contributor Author

Done!! @srikanthccv Please take a look.

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

@pridhi-arora We are almost there. This looks good to me. Just two minor things.

CHANGELOG.md Outdated Show resolved Hide resolved
@pridhi-arora
Copy link
Contributor Author

@srikanthccv Done!

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for addressing all comments.

@srikanthccv srikanthccv marked this pull request as ready for review November 14, 2022 15:38
@pridhi-arora
Copy link
Contributor Author

Thanks, @srikanthccv, for all the help. Do you think I am ready to take this one up now?

@srikanthccv
Copy link
Member

It's assigned to someone else already, but I don't think they are working on it. Let's wait for some time for them to reply; otherwise, I will assign it to you. We can find something else for you if they say they are still working on it. Meanwhile, you can play around with the codebase and browse through the contrib as well https://github.com/open-telemetry/opentelemetry-python-contrib.

@pridhi-arora
Copy link
Contributor Author

pridhi-arora commented Nov 15, 2022

Thanks @srikanthccv. Can you break down https://github.com/open-telemetry/opentelemetry-python-contrib. into parts for me. Which part should I start understanding first? I am asking this so that it doesn't get overwhelming for me.

@ocelotl
Copy link
Contributor

ocelotl commented Nov 17, 2022

This PR has enough approvals now, please fix failing test cases and we can merge 😎

@srikanthccv
Copy link
Member

@pridhi-arora, the lint is still failing. You must reformat your code with black and ensure imports are ordered correctly with isort to get this merged. Please let me know if you need any help with that.

@pridhi-arora
Copy link
Contributor Author

I will try to do it and will come back to you if in case I need your help. Thanks @srikanthccv

@srikanthccv
Copy link
Member

Please fix the conflicts.

@pridhi-arora
Copy link
Contributor Author

@srikanthccv, I have resolved the conflicts. Please take a look.

@pridhi-arora
Copy link
Contributor Author

@srikanthccv The lint check passes.
An error slowed me down on tox -e lint.

I am getting the following on the main branch as well.
I don't know why the CI is not reflecting the following errors.

************* Module opentelemetry-sdk.src.opentelemetry.sdk.metrics._internal.aggregation
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py:19:0: E0611: No name 'inf' in module 'math' (no-name-in-module)
************* Module opentelemetry-sdk.src.opentelemetry.sdk.metrics._internal.exponential_histogram.mapping.exponent_mapping
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exponential_histogram/mapping/exponent_mapping.py:15:0: E0611: No name 'ldexp' in module 'math' (no-name-in-module)
************* Module opentelemetry-sdk.src.opentelemetry.sdk.metrics._internal.exponential_histogram.mapping.logarithm_mapping
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exponential_histogram/mapping/logarithm_mapping.py:15:0: E0611: No name 'exp' in module 'math' (no-name-in-module)
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exponential_histogram/mapping/logarithm_mapping.py:15:0: E0611: No name 'floor' in module 'math' (no-name-in-module)
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exponential_histogram/mapping/logarithm_mapping.py:15:0: E0611: No name 'ldexp' in module 'math' (no-name-in-module)
opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exponential_histogram/mapping/logarithm_mapping.py:15:0: E0611: No name 'log' in module 'math' (no-name-in-module)
************* Module exporter.opentelemetry-exporter-jaeger-thrift.src.opentelemetry.exporter.jaeger.thrift.send
exporter/opentelemetry-exporter-jaeger-thrift/src/opentelemetry/exporter/jaeger/thrift/send.py:74:26: I1101: Module 'math' has no 'ceil' member, but source is unavailable. Consider adding this module to extension-pkg-allow-list if you want to perform analysis based on run-time introspection of living objects. (c-extension-no-member)
exporter/opentelemetry-exporter-jaeger-thrift/src/opentelemetry/exporter/jaeger/thrift/send.py:75:22: I1101: Module 'math' has no 'ceil' member, but source is unavailable. Consider adding this module to extension-pkg-allow-list if you want to perform analysis based on run-time introspection of living objects. (c-extension-no-member)
exporter/opentelemetry-exporter-jaeger-thrift/src/opentelemetry/exporter/jaeger/thrift/send.py:94:27: E1101: Module 'socket' has no 'AF_INET' member (no-member)
exporter/opentelemetry-exporter-jaeger-thrift/src/opentelemetry/exporter/jaeger/thrift/send.py:94:43: E1101: Module 'socket' has no 'SOCK_DGRAM' member (no-member)

------------------------------------------------------------------
Your code has been rated at 9.97/10 (previous run: 9.97/10, +0.00)

@srikanthccv
Copy link
Member

It's probably an issue with your environment only.

@srikanthccv srikanthccv merged commit edf4d6c into open-telemetry:main Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTLP exporters should emit standard user agent string
3 participants