-
Notifications
You must be signed in to change notification settings - Fork 48
docs: add tencent apm as new integration #110
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
Conversation
WalkthroughAdded a new documentation page introducing Tencent APM (TAPM) integration with OpenLLMetry, including description, image, and configuration example for setting tracing endpoint and headers. Notes default TAPM ports (4317 for gRPC) and recommends 55681 for HTTP exporter. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this 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 5d9d4f4 in 2 minutes and 7 seconds. Click for details.
- Reviewed
28lines of code in1files - Skipped
1files when reviewing. - Skipped posting
6draft 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/tencent.mdx:2
- Draft comment:
Possible typo: 'LLM Observability' in the title may be unintended. Consider verifying if it should be 'OpenTelemetry Observability' for consistency with line 8. - 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 title appears intentional - OpenLLMetry seems to be about LLM observability specifically, as evidenced by the product name. The fact that it uses OpenTelemetry as a protocol doesn't mean the title should change. This looks like a documentation mistake by the reviewer, not understanding the product focus. I could be wrong about OpenLLMetry's focus - maybe it is just a general OpenTelemetry tool and the title should be changed. The product name "OpenLLMetry" strongly suggests this is specifically about LLM observability, and the title matches this focus. The mention of OpenTelemetry is about the protocol used, not the product's purpose. The comment should be deleted as it appears to be based on a misunderstanding of the product's focus. The current title accurately reflects the product's purpose.
2. openllmetry/integrations/tencent.mdx:8
- Draft comment:
Add a space between 'APM' and the parenthesis for improved readability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. openllmetry/integrations/tencent.mdx:11
- Draft comment:
Include an alt attribute in thetag for better accessibility.
- 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% According to the rules, we should NOT comment on pure UI/frontend changes. However, accessibility is more than just UI - it's about making content accessible to all users. But the rules also state we should only keep comments that have STRONG EVIDENCE they are correct. Without seeing the actual image, we can't be sure if "Tencent Cloud logo" is even the right alt text. The suggested alt text might not accurately describe the image content - it could be a screenshot or diagram rather than just a logo. Also, accessibility might be handled differently in this documentation framework. While the exact alt text might not be perfect, the fundamental point about needing an alt attribute for accessibility is valid and actionable. The comment should be deleted because it falls under UI/frontend changes which we're instructed to assume the author handled correctly, and we don't have strong evidence that the suggested fix is correct.
4. openllmetry/integrations/tencent.mdx:19
- Draft comment:
Use a standard ASCII hyphen ('-') instead of a non-standard dash in 'URL‑encoded' for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. openllmetry/integrations/tencent.mdx:2
- Draft comment:
The title uses 'OpenLLMetry', but later in the content (line 8) it mentions 'OpenTelemetry'. Please verify if this naming is intentional or if it should be consistent. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. openllmetry/integrations/tencent.mdx:8
- Draft comment:
There appears to be a missing space between 'Tencent APM' and the opening parenthesis. Consider changing to 'Tencent APM (also known asTAPM)' for better readability. - 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% While the suggestion would improve readability slightly, this is an extremely minor formatting issue in documentation. The current format is still perfectly readable and understandable. According to the rules, we should not make purely informative comments or comments about obvious/unimportant issues. The missing space could be considered a legitimate style issue that affects readability. Documentation quality is important. While documentation quality matters, this is such a minor formatting issue that it doesn't warrant a PR comment. The current format doesn't significantly impact readability or understanding. Delete this comment as it addresses an extremely minor formatting issue that doesn't materially impact the documentation's quality or clarity.
Workflow ID: wflow_APc78EPJa9hRdNrJ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
openllmetry/integrations/tencent.mdx (4)
6-6: Prefer a public docs page over a console link.Linking to the console can 404 for readers without access. Consider linking to a TAPM OpenTelemetry integration guide and optionally keeping the console link inline.
8-8: Minor grammar/spacing nit.Add a space before the parenthesis: “Tencent APM (also known as
TAPM) …”.
2-4: Sidebar label: consider “Tencent APM” for consistency.“Tencent Cloud” may be too broad; other integrations typically use the product name. Suggest
sidebarTitle: "Tencent APM".
10-12: Add alt text to the image; asset verifiedAdd an informative alt attribute in openllmetry/integrations/tencent.mdx (lines 10–12). Asset present at img/integrations/tencent.png.
- <img src="/img/integrations/tencent.png" /> + <img src="/img/integrations/tencent.png" alt="OpenLLMetry integration with Tencent APM (TAPM)" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
img/integrations/tencent.pngis excluded by!**/*.png
📒 Files selected for processing (1)
openllmetry/integrations/tencent.mdx(1 hunks)
🔇 Additional comments (1)
openllmetry/integrations/tencent.mdx (1)
14-16: Clarify protocol support and endpoint format for TAPM.Please confirm TAPM accepts OTLP/HTTP ingest on port 4318; many vendor docs show gRPC (4317) examples for TAPM. If HTTP isn’t supported in some regions/editions, call that out and provide the gRPC alternative. (tencentcloud.com)
I can add a dual-protocol block (HTTP and gRPC) once you confirm TAPM’s HTTP support and expected endpoint shape.
| TRACELOOP_BASE_URL="<TAPM_ENDPOINT>" # Use port `55681` rather than `4317` as the default port. | ||
| TRACELOOP_HEADERS="Authorization=Bearer%20<TAPM_TOKEN>" # header values in env variables must be URL‑encoded. | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect OTLP HTTP port (55681 is legacy); use 4318 and include /v1/traces.
OTLP/HTTP’s default port is 4318 (not 55681). 55681 was used historically and is deprecated. Also, for OTLP/HTTP the traces path should be /v1/traces. Recommend updating the example and note accordingly. (opentelemetry.io)
Apply this diff:
-TRACELOOP_BASE_URL="<TAPM_ENDPOINT>" # Use port `55681` rather than `4317` as the default port.
-TRACELOOP_HEADERS="Authorization=Bearer%20<TAPM_TOKEN>" # header values in env variables must be URL‑encoded.
+TRACELOOP_BASE_URL="https://<TAPM_HOST>:4318/v1/traces" # OTLP/HTTP default port is 4318; include /v1/traces.
+TRACELOOP_HEADERS="Authorization=Bearer%20<TAPM_TOKEN>" # header values in env variables must be URL‑encoded.-Tencent APM defaults to using port `4317` for the gRPC exporter; we recommend using port `55681` instead, which is the HTTP exporter port.
+Tencent APM defaults to using port `4317` for OTLP/gRPC. If you use OTLP/HTTP, use port `4318` (the OTLP/HTTP default) and include the `/v1/traces` path.Also applies to: 22-22
🤖 Prompt for AI Agents
In openllmetry/integrations/tencent.mdx around lines 18-20 (and also line 22),
update the OTLP/HTTP example to use the current default port 4318 instead of the
deprecated 55681 and ensure the endpoint includes the traces path /v1/traces
(e.g., TRACELOOP_BASE_URL should point to
http(s)://<TAPM_ENDPOINT>:4318/v1/traces); keep the URL-encoding note for
TRACELOOP_HEADERS but adjust the example values and any accompanying text to
reflect the correct port and path.
nirga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @minimAluminiumalism!
I’m a developer from the Tencent Cloud APM team, and we would like to add our platform as a new integration. @nirga
Important
Adds documentation for integrating Tencent APM with OpenLLMetry, including configuration steps and port recommendations.
tencent.mdxto document integration of Tencent APM with OpenLLMetry.55681instead of4317for HTTP exporter.This description was created by
for 5d9d4f4. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit