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

chore: add instrumentation metadata to package.json #2292

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented Jun 21, 2024

This is the first implementation of open-telemetry/opentelemetry-js#4725 into the contrib repo.

It adds metadata on the OpenTelemetry components that are exported from this npm package (which at the moment is one per package, but not always the case, for example opentelemetry-resource-detector-aws which exports multiple detectors.

For each component, one can currently publish its:

  • name the name of the component as exported from the package. currently not used in this PR but intended to be added to auto-create the code example for each instrumentation.
  • componentType - "instrumentation". In the future we can add metadata to more components and this field can identify if the component is "instrumentation" / "sampler" / "propagator" / resource-detector" etc.
  • platforms - an array with either "node" or "web". instrumentations are always either web or node, but I want to keep it generic so we can mark future non-instrumentation components as both web and node, or add additional platforms if the need arise (like deno).

I have updated the lint-markdown script to use this info directly from the package itself, instead of reading it indirectly from the auto-instrumentation packages dependencies.

After this one merges, I will work on adding and utilizing more metadata as followup PRs as described in the issue.

Copy link

codecov bot commented Jun 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.30%. Comparing base (dfb2dff) to head (43b4977).
Report is 171 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2292      +/-   ##
==========================================
- Coverage   90.97%   90.30%   -0.68%     
==========================================
  Files         146      147       +1     
  Lines        7492     7263     -229     
  Branches     1502     1509       +7     
==========================================
- Hits         6816     6559     -257     
- Misses        676      704      +28     

see 57 files with indirect coverage changes

@blumamir
Copy link
Member Author

@open-telemetry/javascript-approvers would love to get feedback on this approach as an MVP + demonstration of how it can be actually leveraged by a tool (lint readme).

Please feel free to share any concerns or support for documenting the opentelemetry-domain-specific metadata for the components each npm package implements.

I am motivated to work on the followup metadata properties and utilize them, and would be happy to have this one in to build the next steps on top of it

@blumamir
Copy link
Member Author

more on the motivation and future plans can be found in open-telemetry/opentelemetry-js#4725. any feedback is appreciated 🙏

"opentelemetry": {
"components": [
{
"name": "KafkajsInstrumentation",
Copy link
Contributor

@trentm trentm Jun 28, 2024

Choose a reason for hiding this comment

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

I'm hesitant on this change.

  • It adds a bit of maintenance. For example, this KafkajsInstrumentation is out of sync with the code which has a different capitalization KafkaJsInstrumentation.
  • Given this being broken up into an array of components, I understand the desire to have a "name" field for each component. However, that information isn't being used anywhere, so the maintenance of having something that can get out of sync with the code is unfortunate. Do you have a particular use case for the enumeration of these named components? Perhaps some specifics around the "OpenTelemetry control planes" or "Enhancements for UIs" mentioned in #4725 would help.
  • The use of the "plaforms" field is (at least currently) only in scripts/lint-readme.js. However, that lint usage of isNode, isWeb is used for adding a README section about inclusion in auto-instrumentations-node or -web. Isn't the current lint-readme.js querying of auto-instrumentations-node/-web the more accurate information? Recent discussion about some new instrumentations targetting "web" were not going to be included in auto-instrumentations-web, IIRC.

What if we added just this?

"opentelemetry": {
  "targetPlatforms": ["node"]
},

Granted that a package could export components, some of which target web and some of which target node, but I wouldn't think it likely.

(Another key that might be useful is "stability" or "stabilityLevel" using the values defined at https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/CONTRIBUTING.md#component-lifecycle)

At the least, it would be helpful to have a doc section somewhere (GUIDELINES.md seems okay, but currently is scoped on being an instrumentation implementation guide) that explains what this "opentelemetry" package.json key is about (stability, schema) when people ask.

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.

None yet