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 bigtable_table_id attribute to opencensus stats / traces #468

Open
mackenziestarr opened this issue Oct 15, 2020 · 7 comments
Open

add bigtable_table_id attribute to opencensus stats / traces #468

mackenziestarr opened this issue Oct 15, 2020 · 7 comments
Labels
api: bigtable Issues related to the googleapis/java-bigtable API. priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@mackenziestarr
Copy link
Contributor

Is your feature request related to a problem? Please describe.

It is useful to understand the distribution of latency and errors across tables for a given bigtable instance. The opencensus stats already include a lot of helpful attributes but bigtable_table_id would open up a lot of opportunities for debugging.

Describe the solution you'd like

Include bigtable_table_id in the opencensus stats and trace attributes

Describe alternatives you've considered

Not doing it and being a bit disappointed

@product-auto-label product-auto-label bot added the api: bigtable Issues related to the googleapis/java-bigtable API. label Oct 15, 2020
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Oct 16, 2020
@kolea2 kolea2 added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Oct 16, 2020
@dmitry-fa dmitry-fa self-assigned this Oct 22, 2020
@dmitry-fa dmitry-fa removed their assignment Nov 5, 2020
@igorbernstein2
Copy link
Contributor

Hi, sorry for the late reply here. The current design decisions make this a fairly difficult feature to implement. All of the tags that we currently populate are known during client construction time. Table ids are not known until the request is created, which is why they don't currently exist.

@igorbernstein2 igorbernstein2 added the priority: p3 Desirable enhancement or fix. May not be included in next release. label Jan 5, 2021
@mackenziestarr
Copy link
Contributor Author

@igorbernstein2 thanks for the update and getting this prioritized

@rravi-sift
Copy link

Hi @igorbernstein2, bumping up this feature request, we are also interested in this feature since we have many tables in an instance. It looks like BuiltInMetricsTracer supports this feature, could this be extended to MetricsTracer as well?

if (request != null) {
this.tableId = Util.extractTableId(request);
}

@igorbernstein2
Copy link
Contributor

Hi, we will be migrating from opencensus to opentelemetry soon. At that point we will expose the builtin metrics namespace and allow end users to export the builtin metrics to other destinations. Would that address your usecase? or do you still need the ability to integrate deeper in the stack?

@rravi-sift
Copy link

Hi @igorbernstein2,

We chose OpenCensus over the built-in metrics primarily because the latter doesn't allow us to modify metric tags. Specifically, we're keen on tagging metrics with the client's zone and hostname, which is achievable with OpenCensus.

I have a couple of queries regarding this:

  1. If the library transitions to OpenTelemetry, would it simplify the process of customizing or adding new tags? If not, we would not benefit from the migration
  2. We are currently blocked on this issue because we have Bigtable instances with hundreds of tables; hence, the current OpenCensus metric does not paint a good picture. Do you know the ETA for migration?

Thanks for your assistance.

@igorbernstein2
Copy link
Contributor

tldr: yes opentelemetry will make it easy to export metrics that have static labels like the zone & hostname.

Longer answer:

For built-in metrics, we have to aggregate away the client resource labels (hostname, pid, etc) due to an explosion of cardinality. This is the reason why the hostname is not preserved for builtin metrics. In addition, built-in metrics are the only metrics that the bigtable service will collect for free. So we need a private exporter. When implementing built-in metrics, opentelemetry wasnt quite ready, so we implemented it using OpenCensus. This created an issue for us where OpenCensus only has a single global namespace, which prevents us from selectively exporting built-in exports to bigtable service and conversely, it pollutes existing OpenCensus integrations. So we ended up shading & relocating the OpenCensus to achieve a private namespace. Unfortunately having this private namespace prevents you from attaching your own exporter with static tags.

The previous iteration of metrics (the ones that lack table id), uses the global namespace and thus allows you to attach any exporter that you would like. Unfortunately as discussed earlier in this issue, that implementation is currently missing table ids. Unfortunately there is no easy way to add another dimension to existing views without breaking existing applications.

In the current state, the only integration path for custom static labels & table ids is to fork MetricsTracer (by extending BigtableTracer) and define your own meters and integrate with Opentelemetry or OpenCensus.

In the near term we will migrate the BuiltinMetrics to use Opentelemetry which has the ability to define namespaces w/o resorting to shading. This will give endusers access to the builtin metrics namespace and register their own exporters. These exporters can define arbitrary static labels like hostname. Which I believe will address your usecase. In terms of timing, this work has started and their are a couple of outstanding PRs for the migration that need to be landed. So I suspect it will be ready this quarter.

@robwil
Copy link

robwil commented Oct 16, 2024

I wanted to ask what the status of this one is, given that the OpenTelemetry support (#2166) has been added. Is there a way now to add tableId? As far as I can tell, the latest code has tableId available in attributes that are used by BuiltinMetricsTracer, but this is only used for aggregated metrics views, not for individual traces (which still relies on OpencensusTracer - which still seems to rely on baseAttributes where it's not easy to get tableId).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the googleapis/java-bigtable API. priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants