Skip to content

Routing to Dynatrace docs link #72

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shrilekha
Copy link

@shrilekha shrilekha commented Apr 24, 2025

  1. Edited the scopes required for the access token
  2. Since Environment variables aren't recommended by Dynatrace as much as sending in the API URL in the init section, suggesting routing directly to the Dynatrace docs for reference too.

Important

Updates Dynatrace integration docs to modify access token scopes, suggest direct API URL initialization, and add a documentation link.

  • Documentation:
    • Updates required access token scopes in dynatrace.mdx to include events.ingest, metrics.read, and settings.write.
    • Recommends initializing OpenLLMetry with the API URL directly in dynatrace.mdx, replacing environment variable usage.
    • Adds a link to Dynatrace documentation for further reference in dynatrace.mdx.

This description was created by Ellipsis for 055e587. You can customize this summary. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 055e587 in 2 minutes and 23 seconds. Click for details.
  • Reviewed 34 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. openllmetry/integrations/dynatrace.mdx:14
  • Draft comment:
    Ensure that the additional permission scopes (events.ingest, metrics.read, settings.write) are indeed required and documented. Consider adding a brief note on why these extra scopes are needed.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 30% <= threshold 50% The comment is asking the PR author to ensure that the additional permission scopes are required and documented. This falls under asking the author to confirm their intention or ensure something, which is against the rules. However, the suggestion to add a brief note on why these extra scopes are needed is a valid suggestion for improving documentation.
2. openllmetry/integrations/dynatrace.mdx:16
  • Draft comment:
    The section now introduces the API initialization pattern. Confirm that this aligns with best practices recommended by Dynatrace and properly replaces environment variable usage.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to confirm alignment with best practices and proper replacement of environment variable usage. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules.
3. openllmetry/integrations/dynatrace.mdx:14
  • Draft comment:
    Verify that the additional token scopes (events.ingest, metrics.read, settings.write) are required per Dynatrace docs, and consider briefly noting why each is needed.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to verify the necessity of certain token scopes, which violates the rule against asking the author to confirm their intention or ensure behavior. It also suggests noting why each scope is needed, which is more of a suggestion for documentation rather than a code improvement.
4. openllmetry/integrations/dynatrace.mdx:16
  • Draft comment:
    The text mentions 'OpenLLMetry' while the code snippet uses 'Traceloop'. Ensure naming consistency or clarify the relationship between them.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The naming difference appears intentional - OpenLLMetry seems to be the product name while Traceloop is the SDK implementation name. This is a common pattern in software (like React Native vs react-native package). The comment suggests changing documentation to match implementation details, which could actually make things more confusing. The current naming likely helps users understand the relationship between the product and its implementation. I could be wrong about the relationship between OpenLLMetry and Traceloop. There might be a legitimate confusion that needs to be addressed. Even if I'm wrong about the exact relationship, changing the product name in documentation to match implementation details would likely be more confusing than helpful. Documentation should use the product name. The comment should be deleted as it suggests changing clear product documentation to match implementation details, which would likely reduce clarity rather than improve it.
5. openllmetry/integrations/dynatrace.mdx:20
  • Draft comment:
    Advise against hardcoding API tokens in example code; recommend using secure storage (e.g., environment variables or vaults) in production deployments.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a documentation example showing how to structure the API call. Using <YOUR_DT_API_TOKEN> as a placeholder is a common and clear way to show where users should put their token. The suggestion to use environment variables, while good security practice, makes the example more complex and less immediately clear. Documentation often uses placeholders for clarity. The comment does raise a valid security best practice. Some developers might copy-paste the example without thinking about security. However, this is clearly documentation with a placeholder, not production code. The placeholder makes it obvious that users need to replace it with their own token. Delete the comment. The current placeholder approach is appropriate for documentation, and the comment would make the example unnecessarily complex.
6. openllmetry/integrations/dynatrace.mdx:28
  • Draft comment:
    Consider using a more descriptive link text (e.g., 'Dynatrace API Documentation') instead of 'docs link' for better clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
7. openllmetry/integrations/dynatrace.mdx:10
  • Draft comment:
    There is a naming inconsistency: the introduction on line 10 refers to 'OpenTelemetry ingest endpoint' while later (line 16 and header) the product is referred to as 'OpenLLMetry'. Please standardize the naming to avoid confusion.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_57YVNI19xudu7W2P

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

1 participant