Skip to content
This repository has been archived by the owner on Sep 1, 2022. It is now read-only.

feat: include rpc calls and outbound http requests from url parts for… #1308

Merged
merged 10 commits into from
Jul 13, 2022
Merged

feat: include rpc calls and outbound http requests from url parts for… #1308

merged 10 commits into from
Jul 13, 2022

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Jun 23, 2022

… external metircs

Fixes SigNoz/signoz#1305
Fixes SigNoz/signoz#1300
Part SigNoz/signoz#1301

The HTTP semconv recommend setting the one of the following sets of attributes for the client HTTP requests. The client instrumentation may set any one of them. We should take that into account when deciding if the request is external.

  • http.url
  • http.scheme, http.host, http.target
  • http.scheme, http.peer.name, net.peer.port, http.target
  • http.scheme, net.peer.ip, net.peer.port, http.target

And for the RPC systems there should be rpc.{system,service,method} tuple. This is a generic requirement. For instance if you look at the AWS botocore instrumentation which is used for invoking lambdas using it's client these are set along with other FaaS attributes. This covers good number of cases but not the exhaustive list.

@srikanthccv srikanthccv marked this pull request as ready for review June 23, 2022 10:19
@ankitnayan
Copy link
Contributor

@srikanthccv this won't make it work. After detecting the keys like http.url we extract the dimensions from relevant attributes to create labels which are defined at https://github.com/SigNoz/opentelemetry-collector-contrib/blob/develop/processor/signozspanmetricsprocessor/processor.go#L137-L141

As the set of attributes extracted depends on whether the call is rpc or http, we need to either define a common label key that works for both rpc and http or we define different keys for both and use different keys at querying.

According to the above decision, we will have to change our promql at the frontend api call to query_range. We need to link that issue with this so that both are merged for the feature to be effectively seen in the UI.

@srikanthccv
Copy link
Member Author

@ankitnayan good catch. I didn't think about the static queries and labels. Only tested the latency_sum and latency_count. It's probably not good idea to combine everything into one. Then this also requires some new panel in UI for each different category and how they are organized. This turns out to be not as quick as I thought.

ankitnayan
ankitnayan previously approved these changes Jul 12, 2022
ankitnayan
ankitnayan previously approved these changes Jul 13, 2022
makeavish
makeavish previously approved these changes Jul 13, 2022
@makeavish
Copy link
Member

LGTM @ankitnayan

@srikanthccv
Copy link
Member Author

@ankitnayan please wait for me to check things with latest changes and image before merging.

@srikanthccv
Copy link
Member Author

Looks like number can't repeat (now this PR has same as error_index_v2) in the schema .sql file even if the description text is not same.

@makeavish
Copy link
Member

Yes @srikanthccv numbers can't be same in gomigrate, please update numbers to 13 as exceptions PR is already merged.

@srikanthccv srikanthccv dismissed stale reviews from makeavish and ankitnayan via 69fe60e July 13, 2022 14:51
@ankitnayan ankitnayan self-requested a review July 13, 2022 15:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants