Skip to content

Conversation

@maryliag
Copy link
Contributor

@maryliag maryliag commented Mar 28, 2024

Which problem is this PR solving?

Short description of the changes

  • Update README of Instana Resource Detector to clarify the source of the data and add Semantic Conventions

Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

The change is not clear to me.

On the README of the resource detectors, there are no details about which values are being detected and what are their respective keys, making it hard to identify what we have available.
Improve documentation, adding a table with the info for all resource detectors.

I suggest to choose different headlines and put a sentence, which describes it.

@maryliag
Copy link
Contributor Author

what different headline do you suggest? I'm trying to keep consistent with existing docs for other resource detectors, which use "attributes" and "description"

@maryliag maryliag force-pushed the doc-instana branch 3 times, most recently from 0a4040a to a4abb6e Compare March 28, 2024 17:25
@maryliag
Copy link
Contributor Author

Since this is about clarifying which resource attributes are being set by the detectors, and what are their sources, I update to. "Resource Attributes" instead of just "Attributes" and updated the description. Let me know if that is better

@maryliag maryliag force-pushed the doc-instana branch 3 times, most recently from ad376c3 to 445d8a6 Compare March 29, 2024 15:47
@codecov
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Merging #2063 (439fa1b) into main (dfb2dff) will decrease coverage by 0.16%.
Report is 37 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2063      +/-   ##
==========================================
- Coverage   90.97%   90.82%   -0.16%     
==========================================
  Files         146      148       +2     
  Lines        7492     7672     +180     
  Branches     1502     1537      +35     
==========================================
+ Hits         6816     6968     +152     
- Misses        676      704      +28     

see 8 files with indirect coverage changes

Copy link
Contributor

@kirrg001 kirrg001 left a comment

Choose a reason for hiding this comment

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

I still don't agree with this table for the public README.

What will a customer do with this table?
The new table you have added is IMO internal logic and should not belong in the public docs. If I am wrong, please explain to me.

The interesting attributes for the customers are:

  • the two env variables, which are already documented
  • and [SemanticResourceAttributes.SERVICE_NAME]: "TestService" (documented as well)

The detector reads the process.pid from the process and additionally returns the agentUuid based on the env variables (which isservice.instance.id).

@maryliag
Copy link
Contributor Author

maryliag commented Apr 2, 2024

Currently, there is no documentation on what values are being set as the resources, this means that if a user wants to know what options they have available, so they can add filters, different types of charts, and so on, the only way of knowing what they have is to look into the actual code or start sending metrics/traces and check the values there, which is not ideal.
This table is an easy way for the user to know what values are there and where they come from, so they can use accordingly.

This table exists for other README, but was missing for others, this is why I created PRs to add to the missing ones. This is a matter of keeping the documentation consistent in all READMEs.

This also helps by showing what users can have by default, so they can also add anything that is no there. For example, the SERVICE_NAME that you mentioned doesn't come by default, only if they add it. Instead of them trying to figure out if they need to add something or not, they can check the table and add anything else that is not there by default, which on Instana case is just 2 values.

Examples: Alibaba and Azure

@maryliag maryliag force-pushed the doc-instana branch 2 times, most recently from 34f4b45 to 432e1a2 Compare April 3, 2024 13:49
@maryliag
Copy link
Contributor Author

maryliag commented Apr 3, 2024

@kirrg001 I tagged also the issue #2025 here, that is about adding the semantic conventions used on the README, so the 2 issues are aligned.
The semantinc conventions for this package got updated on #2051, so I'm still waiting for that to get approved/merged and this one can be merged after it

@maryliag maryliag changed the title chore: update readme for instana resource detector docs(resource-detector-instana): update readme for instana resource detector Apr 3, 2024
@kirrg001
Copy link
Contributor

kirrg001 commented Apr 4, 2024

This table is an easy way for the user to know what values are there and where they come from, so they can use accordingly.

I am not against the table, I just don't agree with how the table is presented and described.

This table exists for other README, but was missing for others, this is why I created PRs to add to the missing ones. This is a matter of keeping the documentation consistent in all READMEs.

I'll just approve then.

@trentm trentm merged commit 13b49b3 into open-telemetry:main Apr 9, 2024
@maryliag maryliag deleted the doc-instana branch April 10, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.