Adding PHP Dynamic instrumentation preview details to docs #27963
Conversation
sstonehill12
added
WORK IN PROGRESS
Preview links (active after the
|
Updated minimum version and added limitations section
| ### Supported features | ||
|
|
||
| - Dynamic Logs, Metrics, Spans, and Span Tags | ||
| - Dynamic Logs attached to a function/method |
There was a problem hiding this comment.
we only mention function/method for attached logs. but I assume this is for all probe types?
If so, this line and line "Dynamic Logs attached to a specific file/line" at the unsupported features is confusing.
There was a problem hiding this comment.
@shatzi Let me know if the updated version is better!
There was a problem hiding this comment.
hi @sstonehill12 thanks for adding this! Left a small suggestion :)
| @@ -35,6 +35,24 @@ Configure Dynamic Instrumentation using the following environment variables: | |||
|
|
|||
| See [Dynamic Instrumentation][5] for information about adding instrumentations and browsing and indexing the data. | |||
|
|
|||
| ## Limitations | |||
There was a problem hiding this comment.
| ## Limitations |
I would suggest to remove this as it's a little confusing with a heading "limitations" followed by a heading of Supported Features, as a reader it's unclear
There was a problem hiding this comment.
This goes against the design of Node.js and Ruby pages, which both have a "Limitations" section.
I don't personally think this headline is a problem, and I like the ability to deep link to it which can come in handy in support cases. It's also nice to have it show up the the ToC.
I see that we have the following options:
- Keep the headline (I don't find it confusing)
- Rename the headline, so it's less confusion
- Change the wording of the sentence following the headline, so it's less confusion
- Remove the headline in Node.js and Ruby as well
There was a problem hiding this comment.
Let me know what you all decide and if you need a re-review after :)
There was a problem hiding this comment.
@watson Good points about the deep link and ToC. I'll revert the change back to "Limitations" and we can think about a better headline for this section and apply it across all the docs in a separate PR (if we come up with one).
Co-authored-by: Alicia Scott <[email protected]>
|
@aliciascott Suggested changes were added! I'll submit a separate PR to apply the same formatting changes to the other DI language enablement pages for consistency's sake. |
|
/merge |
|
View all feedbacks in Devflow UI.
This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
devflow unqueued this merge request: It did not become mergeable within the expected time |
dd-devflow
bot
added
mergequeue-status: waiting
mergequeue-status: removed
and removed
mergequeue-status: waiting
labels
|
/merge |
|
View all feedbacks in Devflow UI.
This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
The median merge time in
|
dd-devflow
bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
Co-authored-by: watson <[email protected]>
What does this PR do? What is the motivation?
Update Dynamic Instrumentation docs to include the PHP support preview.
Merge instructions
Merge readiness:
Merge queue is enabled in this repo. To have it automatically merged after it receives the required reviews, create the PR (from a branch that follows the
<yourname>/descriptionnaming convention) and then add the following PR comment:Additional notes