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

Refactor Metric object format #2646

Closed
ocelotl opened this issue Apr 27, 2022 · 2 comments · Fixed by #2658
Closed

Refactor Metric object format #2646

ocelotl opened this issue Apr 27, 2022 · 2 comments · Fixed by #2658
Assignees
Labels
1.10.0rc1 release candidate 1 for metrics GA metrics sdk Affects the SDK package.

Comments

@ocelotl
Copy link
Contributor

ocelotl commented Apr 27, 2022

Hmm im wondering if we should implement this grouping in the Metric exporter format (a breaking change) before we send it to the exporter.. Originally, I was thinking the flat representation would be easy to work with but it looks like the OTLP and Prom exporters are having to duplicate a lot of grouping logic.

We could make it look more like the OTLP structure

Originally posted by @aabmass in #2639 (comment)

more context

Just for clarity, this what our current structure looks like in JSON:

[
  {
    "attributes": {"foo": "bar"},
    "description": "hello",
    "instrumentation_scope": {"name": "mymeter", ...},
    "name": "myview",
    "resource": {"service.name": "foo", ...},
    "unit": "s",
    "point": {
      "aggregation_temporality": "CUMULATIVE",
      "is_monotonic": false,
      "start_time_unix_nano": 123456,
      "time_unix_nano": 123457,
      "value": 123.3
    }
  },
  {
    "attributes": {"foo": "fab"},
    "description": "hello",
    "instrumentation_scope": {"name": "mymeter", ...},
    "name": "myview",
    "resource": {"service.name": "foo", ...},
    "unit": "s",
    "point": {
      "aggregation_temporality": "CUMULATIVE",
      "is_monotonic": false,
      "start_time_unix_nano": 123456,
      "time_unix_nano": 123457,
      "value": 123.3
    }
  }
]

Notice there is a single point per entry and minimal nesting. However, instrumentation_scope, resource, and some other stuff is present on every message where it could be grouped. Compare this to OTLP where things are nested and grouped:

   
{
  "resourceMetrics": [
    {
      "resource": {},
      "instrumentationLibraryMetrics": [
        {
          "instrumentationLibrary": {},
          "metrics": [
            {
              "name": "testcounter",
              "description": "This is a test counter",
              "unit": "1",
              "sum": {
                "dataPoints": [
                  {
                    "attributes": [
                      {
                        "key": "foo",
                        "value": {
                          "stringValue": "bar"
                        }
                      }
                    ],
                    "startTimeUnixNano": "1634322229906722370",
                    "timeUnixNano": "1634322234906722370",
                    "asInt": "253"
                  }
                ],
                "aggregationTemporality": "AGGREGATION_TEMPORALITY_CUMULATIVE",
                "isMonotonic": true
              }
            }
          ]
        }
      ]
    }
  ]
}

The Python OTLP exporter has to do this grouping to get from our current structure to the real OTLP format. Prometheus has to do something similar. The first representation is closer to how things look in-memory for the metrics SDK.

Should we do this grouping and make the structure we expose to exporters closer to OTLP?

@srikanthccv
Copy link
Member

fwiw other sigs have followed this approach of keeping the class/object model close to OTLP data model ex: open-telemetry/opentelemetry-js#2809 and dotnet/others appear to be doing same.

@aabmass
Copy link
Member

aabmass commented Apr 28, 2022

Thanks @srikanthccv. I think the only downside to grouping is it's doing extra work, whereas the current structure is basically exactly what we have in memory. I'm OK to do the grouping in our code. The code is basically implemented already in the OTLP exporter

The only weird thing is our Aggregation interface returns instances of Metric and we use that internally a few spots. I suppose we can make that internal and then create a separate dataclass for the Metric object that exporters receive.

@ocelotl ocelotl self-assigned this Apr 28, 2022
@ocelotl ocelotl changed the title Hmm im wondering if we should implement this grouping in the Metric exporter format (a breaking change) before we send it to the exporter.. Originally, I was thinking the flat representation would be easy to work with but it looks like the OTLP and Prom exporters are having to duplicate a lot of grouping logic. Refactor Metric object format Apr 29, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 4, 2022
@ocelotl ocelotl added 1.10.0rc1 release candidate 1 for metrics GA sdk Affects the SDK package. metrics labels May 4, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 6, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 7, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 8, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 10, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 10, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 10, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 10, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 10, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 10, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 10, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 11, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 11, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 11, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 11, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 11, 2022
ocelotl added a commit to ocelotl/opentelemetry-python that referenced this issue May 11, 2022
ocelotl added a commit that referenced this issue May 12, 2022
* Refactor metric format

Fixes #2646

* Do not overwrite pb2_scope_metrics

* Refactor for loops

* Add multiple scope test case

* Fix interfaces

* Fix docs

* Update exporter/opentelemetry-exporter-prometheus/src/opentelemetry/exporter/prometheus/__init__.py

Co-authored-by: Aaron Abbott <[email protected]>

* Fix lint

* Remove resource check

* Remove instrumentation_scope check

* Group metrics by instrumentation scopes in the SDK

* Remove label_keyss

* Use strings instead of mocks

* Return generator instead of a list

* Fix lint

* Rename variables

Co-authored-by: Aaron Abbott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.10.0rc1 release candidate 1 for metrics GA metrics sdk Affects the SDK package.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants