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

fix(instrumentation): replace api base url hardcoded value from watsonx #2173

Conversation

rajmanna-dev
Copy link

Fixed #2163

I am a beginner in learning python, I don't know how to make or add test for this changes. I need guidence.

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Oct 18, 2024
@CLAassistant
Copy link

CLAassistant commented Oct 18, 2024

CLA assistant check
All committers have signed the CLA.

@dosubot dosubot bot added the python Pull requests that update Python code label Oct 18, 2024
Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

Thanks @rajmanna-dev! Left a small comment. Can you sign the CLA?

packages/sample-app/sample_app/watsonx-langchain.py Outdated Show resolved Hide resolved
Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

@rajmanna-dev note that CI is failing - I think it's related to the import you added

@@ -14,6 +14,7 @@
from opentelemetry.trace.status import Status, StatusCode
from opentelemetry.metrics import get_meter
from opentelemetry.metrics import Counter, Histogram
from sample-app.sample_app.watsonx-langchain import watsonx_llm_init
Copy link
Member

Choose a reason for hiding this comment

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

remove?

@@ -101,10 +102,15 @@ def _set_span_attribute(span, name, value):


def _set_api_attributes(span):
watsonx_llm = watsonx_llm_init()
Copy link
Member

Choose a reason for hiding this comment

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

why do you need this?

Copy link
Author

@rajmanna-dev rajmanna-dev Oct 18, 2024

Choose a reason for hiding this comment

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

How can i extract url directly from WatsonxLLM objcet.

Copy link
Member

Choose a reason for hiding this comment

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

See the comment here I shared an example of how we've done it elsewhere - #2165 (review)

Copy link
Author

Choose a reason for hiding this comment

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

Should i check the attribute 'url' of the instance and then set that to api_base_url variable.
Like:

if hasattr(instance, "url"):
    api_base_url = instance.url

Copy link
Member

Choose a reason for hiding this comment

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

Have you checked that the instance can actually have this attribute? I'd run one of the sample apps and see

packages/sample-app/sample_app/watsonx-langchain.py Outdated Show resolved Hide resolved
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Oct 18, 2024
@rajmanna-dev rajmanna-dev deleted the fix-2163-replace-api-base-url-hardcoded-value branch October 18, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests that update Python code size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug Report: Watsonx hardcoded values
3 participants