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

Fix naming inconsistency of telemetry.metrics.common.attributes.router #2116

Merged
merged 9 commits into from
Nov 23, 2022

Conversation

BrynCooke
Copy link
Contributor

@BrynCooke BrynCooke commented Nov 16, 2022

Fix naming inconsistency of telemetry.metrics.common.attributes.router (Issue #2076)

Mirroring the rest of the config router should be supergraph

telemetry:
  metrics:
    common:
      attributes:
        router: # old

becomes

telemetry:
  metrics:
    common:
      attributes:
        supergraph: # new

Configuration upgrades (Issue #2123)

Occasionally we will make changes to the Router yaml configuration format.
When starting the Router if the configuration can be upgraded it will do so automatically and display a warning:

2022-11-22T14:01:46.884897Z  WARN router configuration contains deprecated options: 

  1. telemetry.tracing.trace_config.attributes.router has been renamed to 'supergraph' for consistency

These will become errors in the future. Run `router config upgrade <path_to_router.yaml>` to see a suggested upgraded configuration.

Note: If a configuration has errors after upgrading then the configuration will not be upgraded automatically.

From the CLI users can run:

  • router config upgrade <path_to_router.yaml> to output configuration that has been upgraded to match the latest config format.
  • router config upgrade --diff <path_to_router.yaml> to output a diff e.g.
 telemetry:
   apollo:
     client_name_header: apollographql-client-name
   metrics:
     common:
       attributes:
-        router:
+        supergraph:
           request:
             header:
             - named: "1" # foo

There are situations where comments and whitespace are not preserved. This may be improved in future.

By @bryncooke in #2116

CLI structure changes (Issue #2123)

As the Router gains functionality the limitations of the current CLI structure are becoming apparent.

There is now a separate subcommand for config related operations:

  • config
    • schema - Output the configuration schema
    • upgrade - Upgrade the configuration with optional diff support.

router --schema has been deprecated and users should move to router config schema.

Mirroring the rest of the config `router` should be `supergraph`

```yaml
telemetry:
  metrics:
    common:
      attributes:
        router: # old
```
becomes
```yaml
telemetry:
  metrics:
    common:
      attributes:
        supergraph: # new
```

Fixes #2076
@BrynCooke BrynCooke self-assigned this Nov 16, 2022
@BrynCooke BrynCooke requested review from a team, SimonSapin and bnjjj and removed request for a team November 16, 2022 09:56
Copy link
Contributor

@bnjjj bnjjj left a comment

Choose a reason for hiding this comment

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

I think we should still support the old way

Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

this looks good to me!

@EverlastingBugstopper
Copy link
Contributor

EverlastingBugstopper commented Nov 22, 2022

one thing we might do is support stdin for file argument types. In Rover we have a type called FileDescriptorType that implements this behavior.

(usage could be rover supergraph compose --config supergraph.yaml | router -s -)

@EverlastingBugstopper
Copy link
Contributor

this PR introduces -s/--supergraph for the schema. rover uses -s/--schema everywhere for schemas. however, we never refer to a supergraph schema directly in input arguments though. 🤷🏼 food for thought

README.md Outdated
-s, --supergraph <SUPERGRAPH_PATH>
Schema location relative to the project directory [env: APOLLO_ROUTER_SUPERGRAPH_PATH=]

--schema
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is wrong

@BrynCooke
Copy link
Contributor Author

-s/--supergraph has been the name forever. Unfortunately we can't align from the router side for now as we were using --schema for outputting the schema.

We should discuss maybe aligning from the rover side. Let's also check out the terminology studio uses.

@BrynCooke BrynCooke merged commit 655689a into dev Nov 23, 2022
@BrynCooke BrynCooke deleted the bryn/telemetry-metrics-config branch November 23, 2022 15:30
BrynCooke pushed a commit that referenced this pull request Nov 25, 2022
Issue was introduced with #2116 but no release had this in.

Move operations would insert data in the config due to the delete magic value always getting added. Now we check before adding such values.

We may need to move to fluvio-jolt longer term.
@BrynCooke BrynCooke mentioned this pull request Nov 25, 2022
BrynCooke pushed a commit that referenced this pull request Nov 25, 2022
Issue was introduced with #2116 but no release had this in.

Move operations would insert data in the config due to the delete magic value always getting added. Now we check before adding such values.

We may need to move to fluvio-jolt longer term.
BrynCooke added a commit that referenced this pull request Nov 28, 2022
Issue was introduced with #2116 but no release had this in.

Move operations would insert data in the config due to the delete magic
value always getting added. Now we check before adding such values.

We may need to move to fluvio-jolt longer term.

<!--
First, 🌠 thank you 🌠 for considering a contribution to Apollo!

Some of this information is also included in the /CONTRIBUTING.md file
at the
root of this repository.  We suggest you read it!

  https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md

Here are some important details to keep in mind:

* ⏰ Your time is important
To save your precious time, if the contribution you are making will
take more than an hour, please make sure it has been discussed in an
        issue first. This is especially true for feature requests!

* 💡 Features
Feature requests can be created and discussed within a GitHub Issue.
Be sure to search for existing feature requests (and related issues!)
prior to opening a new request. If an existing issue covers the need,
please upvote that issue by using the 👍 emote, rather than opening a
        new issue.

* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.

* 📖 Contribution guidelines
Follow https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
        tests for all new behavior.

* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
        your pull request is meant to accomplish. Provide 🔗 links 🔗 to
associated issues! Documentation in the docs/ directory should be
updated
        as necessary.  Finally, a /CHANGELOG.md entry should be added.

We hope you will find this to be a positive experience! Contribution can
be
intimidating and we hope to alleviate that pain as much as possible.
Without
following these guidelines, you may be missing context that can help you
succeed
with your contribution, which is why we encourage discussion first.
Ultimately,
there is no guarantee that we will be able to merge your pull-request,
but by
following these guidelines we can try to avoid disappointment.

-->

Co-authored-by: bryn <[email protected]>
garypen pushed a commit that referenced this pull request Nov 30, 2022
#2116)

### Fix naming inconsistency of
telemetry.metrics.common.attributes.router ([Issue
#2076](#2076))

Mirroring the rest of the config `router` should be `supergraph`

```yaml
telemetry:
  metrics:
    common:
      attributes:
        router: # old
```
becomes
```yaml
telemetry:
  metrics:
    common:
      attributes:
        supergraph: # new
```

### Configuration upgrades ([Issue
#2123](#2123))

Occasionally we will make changes to the Router yaml configuration
format.
When starting the Router if the configuration can be upgraded it will do
so automatically and display a warning:

```
2022-11-22T14:01:46.884897Z  WARN router configuration contains deprecated options: 

  1. telemetry.tracing.trace_config.attributes.router has been renamed to 'supergraph' for consistency

These will become errors in the future. Run `router config upgrade <path_to_router.yaml>` to see a suggested upgraded configuration.
```

Note: If a configuration has errors after upgrading then the
configuration will not be upgraded automatically.

From the CLI users can run:
* `router config upgrade <path_to_router.yaml>` to output configuration
that has been upgraded to match the latest config format.
* `router config upgrade --diff <path_to_router.yaml>` to output a diff
e.g.
```
 telemetry:
   apollo:
     client_name_header: apollographql-client-name
   metrics:
     common:
       attributes:
-        router:
+        supergraph:
           request:
             header:
             - named: "1" # foo
```

There are situations where comments and whitespace are not preserved.
This may be improved in future.

By [@BrynCooke](https://github.com/bryncooke) in
#2116


### CLI structure changes ([Issue
#2123](#2123))

As the Router gains functionality the limitations of the current CLI
structure are becoming apparent.

There is now a separate subcommand for config related operations:
* `config`
  * `schema` - Output the configuration schema
  * `upgrade` - Upgrade the configuration with optional diff support.

`router --schema` has been deprecated and users should move to `router
config schema`.


<!--
First, 🌠 thank you 🌠 for considering a contribution to Apollo!

Some of this information is also included in the /CONTRIBUTING.md file
at the
root of this repository.  We suggest you read it!

  https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md

Here are some important details to keep in mind:

* ⏰ Your time is important
To save your precious time, if the contribution you are making will
take more than an hour, please make sure it has been discussed in an
        issue first. This is especially true for feature requests!

* 💡 Features
Feature requests can be created and discussed within a GitHub Issue.
Be sure to search for existing feature requests (and related issues!)
prior to opening a new request. If an existing issue covers the need,
please upvote that issue by using the 👍 emote, rather than opening a
        new issue.

* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.

* 📖 Contribution guidelines
Follow https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
        tests for all new behavior.

* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
        your pull request is meant to accomplish. Provide 🔗 links 🔗 to
associated issues! Documentation in the docs/ directory should be
updated
        as necessary.  Finally, a /CHANGELOG.md entry should be added.

We hope you will find this to be a positive experience! Contribution can
be
intimidating and we hope to alleviate that pain as much as possible.
Without
following these guidelines, you may be missing context that can help you
succeed
with your contribution, which is why we encourage discussion first.
Ultimately,
there is no guarantee that we will be able to merge your pull-request,
but by
following these guidelines we can try to avoid disappointment.

-->

Co-authored-by: bryn <[email protected]>
garypen pushed a commit that referenced this pull request Nov 30, 2022
Issue was introduced with #2116 but no release had this in.

Move operations would insert data in the config due to the delete magic
value always getting added. Now we check before adding such values.

We may need to move to fluvio-jolt longer term.

<!--
First, 🌠 thank you 🌠 for considering a contribution to Apollo!

Some of this information is also included in the /CONTRIBUTING.md file
at the
root of this repository.  We suggest you read it!

  https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md

Here are some important details to keep in mind:

* ⏰ Your time is important
To save your precious time, if the contribution you are making will
take more than an hour, please make sure it has been discussed in an
        issue first. This is especially true for feature requests!

* 💡 Features
Feature requests can be created and discussed within a GitHub Issue.
Be sure to search for existing feature requests (and related issues!)
prior to opening a new request. If an existing issue covers the need,
please upvote that issue by using the 👍 emote, rather than opening a
        new issue.

* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.

* 📖 Contribution guidelines
Follow https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
        tests for all new behavior.

* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
        your pull request is meant to accomplish. Provide 🔗 links 🔗 to
associated issues! Documentation in the docs/ directory should be
updated
        as necessary.  Finally, a /CHANGELOG.md entry should be added.

We hope you will find this to be a positive experience! Contribution can
be
intimidating and we hope to alleviate that pain as much as possible.
Without
following these guidelines, you may be missing context that can help you
succeed
with your contribution, which is why we encourage discussion first.
Ultimately,
there is no guarantee that we will be able to merge your pull-request,
but by
following these guidelines we can try to avoid disappointment.

-->

Co-authored-by: bryn <[email protected]>
@BrynCooke BrynCooke added this to the v1.5.0 milestone Dec 3, 2022
@garypen garypen added this to the v1.5.0 milestone Dec 5, 2022
@BrynCooke BrynCooke modified the milestone: v1.5.0 Dec 5, 2022
@BrynCooke BrynCooke mentioned this pull request Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants