langchain instrumentor PR #448
Conversation
- removing unused imports changing != to is not removing f strings with no assignment removing double defined functions
- change camel to snake case - added headers
…logic for init file
…t due to lack of AWS credentials
| @@ -82,6 +82,14 @@ dependencies = [ | |||
| "opentelemetry-instrumentation-urllib3 == 0.54b1", | |||
| "opentelemetry-instrumentation-wsgi == 0.54b1", | |||
| "opentelemetry-instrumentation-cassandra == 0.54b1", | |||
| "langchain == 0.3.27", | |||
There was a problem hiding this comment.
We should not need some of these dependencies here.
For example, why do we need duckduckgo-search?
| @@ -95,6 +103,9 @@ test = [] | |||
| [project.entry-points.opentelemetry_configurator] | |||
| aws_configurator = "amazon.opentelemetry.distro.aws_opentelemetry_configurator:AwsOpenTelemetryConfigurator" | |||
|
|
|||
| [project.entry-points.opentelemetry_instrumentor] | |||
| langchain = "amazon.opentelemetry.distro.instrumentation.mcp.instrumentation:McpInstrumentor" | |||
There was a problem hiding this comment.
Typo?
amazon.opentelemetry.distro.instrumentation.mcp.instrumentation:McpInstrumentor
There was a problem hiding this comment.
Why do we have a pyproject.toml defined here but we are also adding dependencies in the ADOT root pyproject.toml file?
There was a problem hiding this comment.
I think this one can be deleted. I think we had it for the previous structure and I forgot to remove it.
There was a problem hiding this comment.
Why do we need this file? Wouldn't this define ADOT's version to be "0.1.0"?
There was a problem hiding this comment.
Since we're adding the langchain instrumentation to ADOT, I don't think we need to define a version for the langchain instrumentor since it is not really a standalone package anymore.
There was a problem hiding this comment.
What are all these cassette files for? Do we need these?
There was a problem hiding this comment.
no, can be deleted, these are generated from the old tests that would make API calls, will delete.
There was a problem hiding this comment.
Why do we need test-requirements.txt when we already have a pyproject.toml file?
dev-requirements.txt
Outdated
|
Mostly left comments about clean up and dependency related stuff so far. Will take look at the instrumentation logic in the next round of review. |
…essary pyproject and requirements files). removed unnecessary version file, modified tests to skip linter and removed unused vars
…o have the linter skip a line of code
dev-requirements.txt
Outdated
There was a problem hiding this comment.
This file can probably be removed
There was a problem hiding this comment.
I think the dev-requirements.txt file is from before me, are you sure I should delete it?
There was a problem hiding this comment.
Ah my bad, ignore this comment. I thought this was still the old requirements.txt file from before.
There was a problem hiding this comment.
Do we still need a version for our langchain instrumentor if it's just a part of ADOT now?
| "pytest-asyncio == 0.21.0", | ||
| "pytest-vcr == 1.0.2", |
There was a problem hiding this comment.
I don't think we should add these in the main dependencies since they are just for testing. For example, we don't like pytest here either.
| "langchain-core == 0.3.72", | ||
| "langchain-aws == 0.2.15", | ||
| "langchain-community == 0.3.27", | ||
| "langgraph == 0.6.3", |
There was a problem hiding this comment.
For instrumentation libraries, we normally don't add dependencies for the library we're instrumenting directly. For example, see the pyproject.toml in the upstream instrumentor.
The reason for this is because we only want to instrument a customer's library if they have it installed. Otherwise, our instrumentation logic will just not run.
…ts and deleted unnecessary dependencies from pyproject.toml and added new path to pyproject.toml
Migrated code from https://github.com/yiyuan-he/opentelemetry-langchain-instrumentation-v2 to this repo.
Made a new folder in root directory for this code. Because this is standalone from the rest of ADOT, therefore it felt more correct to keep this out as a standalone folder.
Longterm:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.