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

Add pre-massaged data into jinja/jq context when generating registry #204

Closed
lmolkova opened this issue Jun 10, 2024 · 0 comments · Fixed by #246
Closed

Add pre-massaged data into jinja/jq context when generating registry #204

lmolkova opened this issue Jun 10, 2024 · 0 comments · Fixed by #246

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Jun 10, 2024

When generating code, in many cases the JQ part would be similar between different languages.

It particular:

  1. grouping attributes by the root namespace (note: id of the group in the registry is not always accurate - only the attribute name is ).

    Here's the JQ

    .groups
    | map(select(.type == "attribute_group"))
    | map(select(.id | startswith("registry")))
    | map({attributes: .attributes
        | map(.group_id= (if .name |index(".") then .name |split(".")[0] else "other" end))
    })
    | [.[].attributes[] ]
    | group_by(.group_id)
    | map({ id: .[0].group_id, attributes: [.[]] })

    It can be implemented once in rust and appear in the Jinja context as an additional ctx.grouped_attributes property - nobody would need to write and debug it again.

    Same with metrics, events, etc.

  2. Implement the outcome of Code generation: how to avoid naming collisions semantic-conventions#1118

    E.g. Option 3.5 implementation looks like

    | map({
      id: .id,
      attributes: .attributes
        | map(if has("deprecated") then . else . + {"deprecated": null} end)
        | group_by(.const_name)
        | map({const_name: .[0].const_name, names: [.[]] | sort_by(.deprecated) })
        | map(.names[0]),

    I'm not sure how far we can go there, but it could be that ctx.grouped_attributes from p1 above provides all the default behavior we want codegen to follow and resolves collisions in a common way. Someone can still use raw groups and massage data with JQ if they want to.

  3. Minor: sort attributes in ctx.grouped_attributes by name. Requirement level does not appear in attribute definitions code, sorting by requirement level does not make sense there. If someone needs to sort differently, they still could

Ideal outcome would be that language SIGs won't need to write any JQ and would only do something complicated to implement non-standard behavior.

As a first step, it'd be great to reduce https://github.com/lmolkova/opentelemetry-python/blob/5d151671ff9ad5ecb20c507ddfe64684a46aa6c6/scripts/semconv/templates/registry/weaver.yaml#L1-L31 to something like

params:
  excluded: ["ios", "aspnetcore", "signalr", "android", "dotnet", "jvm", "kestrel"]
  stable_package_name: opentelemetry.semconv


templates:
  - pattern: semantic_attributes.j2
    filter: >
      .grouped_attributes
     # filter based on stability - could be more convenient
      | map(select(if $filter == "any" then true else .stability == $filter end)) }) 
     # filter based on excluded namespaces - could be more convenient
      | map(select(.root_namespace as $id | any( $excluded[]; . == $id) | not )) 
      | map(select(.attributes | length > 0))
      | map({
        root_namespace,
        attributes,
        output: $output + "attributes/",
        stable_package_name: $stable_package_name + ".attributes",
        filter: $filter
      })
@lmolkova lmolkova changed the title Add pre-massaged data into jinja context when generating registry Add pre-massaged data into jinja/jq context when generating registry Jun 10, 2024
@lquerel lquerel linked a pull request Jul 17, 2024 that will close this issue
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 a pull request may close this issue.

1 participant