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

Initial implementation #1

Merged
merged 13 commits into from
Mar 5, 2024
Merged

Initial implementation #1

merged 13 commits into from
Mar 5, 2024

Conversation

dhontecillas
Copy link
Contributor

@dhontecillas dhontecillas commented Jan 3, 2024

Please, take a look at the README.md for information about how to configure the library, and at implementation details to have an initial explanation of the packages that compose the library.

The provided example is currently working, and easy to check the reported traces, however some work must be done, to create some dashboard, and check the metrics.

Also, there are still pending tests to be written.

But this implementation is ready to start receiving some feedback.

README.md Outdated Show resolved Hide resolved

In the configuration we find the following root entries:

- `service_name`: to provide a custom name for this service. However if there is
Copy link
Member

Choose a reason for hiding this comment

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

Yo optaría por hacerlo al revés. Si service_name está definido se usa, si no existe se usa el ServiceConfig.Name y si tampoco existe, un default tipo KrakenD.

README.md Outdated

At the router level we have 3 main options:

- `metrics`: boolean to enable / disable if we want to report metrics for this layer
Copy link
Member

Choose a reason for hiding this comment

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

En opencensus usabamos lo contrario. disable_metrics o disable_traces para que el default de la struct sea que esté activado (que entiendo es lo que nos interesa).

return nil, err
}

if cfg.Layers == nil {
Copy link
Member

Choose a reason for hiding this comment

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

If we customize a layer, the rest will be disabled, that can be confusing since not setting anything enables everything by default. May be, each layer should be checked independently. We can also reverse the option so the default struct value (false) mimics this behaviour.

}
if ss, ok := i.(SpanExporter); ok && ss != nil {
s[name] = ss
} else if mm, ok := i.(MetricReader); ok {
Copy link
Member

Choose a reason for hiding this comment

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

mm can't be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should not happen, but since we are not sure, I will add the check as in line 91 , good catch!

@kpacha
Copy link
Member

kpacha commented Jan 10, 2024

I'd rather move the skip_path option to the router instead of having it at the backend, so a set of pipes could be ignored

deepsource-autofix bot added a commit that referenced this pull request Jan 11, 2024
This commit fixes the style issues introduced in 73d0aff according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Jan 12, 2024
This commit fixes the style issues introduced in 52885ad according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Jan 13, 2024
This commit fixes the style issues introduced in 2f3fb7b according to the output
from Go fmt and Gofumpt.

Details: #1
README.md Outdated

```json
"exporters": {
"local_prometheus": {
Copy link
Member

Choose a reason for hiding this comment

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

Thinking in external tools and config validation, I would rather avoid having a "free text" key, and convert exporters to an array instead. Something like:

"exporters": [
{
  "name": "my_prometheus_fashion_Exporter",
  "kind": "prometheus",
  "config": {
     "port": 9090,
     "process_metrics": true,
     "go_metrics": true
   }
 },
....
]```

deepsource-autofix bot added a commit that referenced this pull request Jan 19, 2024
This commit fixes the style issues introduced in c19d3ae according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Jan 20, 2024
This commit fixes the style issues introduced in c20a107 according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Jan 21, 2024
This commit fixes the style issues introduced in e724ddb according to the output
from Go fmt and Gofumpt.

Details: #1
@dhontecillas
Copy link
Contributor Author

A better format for the exporters would be:

exporters": {
                    "prometheus": [
                        {
                            "name": "remote_prometheus",
                            "port": 9092,
                            "process_metrics": true,
                            "go_metrics": true
                        },
                        {
                            "name": "local_prometheus",
                            "port": 9092,
                            "process_metrics": true,
                            "go_metrics": true
                        }]
                }

s := make(map[string]SpanExporter)
var errList []error

ctx := context.Background()

Choose a reason for hiding this comment

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

nit, we should take context from params

@@ -0,0 +1,7 @@
# TODO

Choose a reason for hiding this comment

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

shutting down meter provider gracefully is also TODO

deepsource-autofix bot added a commit that referenced this pull request Jan 29, 2024
This commit fixes the style issues introduced in 58b2a24 according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Jan 29, 2024
This commit fixes the style issues introduced in 3810d73 according to the output
from Go fmt and Gofumpt.

Details: #1
@Harnoor7
Copy link

Harnoor7 commented Feb 2, 2024

@dhontecillas any reason for not using otel-go net/http standard instrumentation support library?

@dhontecillas
Copy link
Contributor Author

dhontecillas commented Feb 2, 2024

@dhontecillas any reason for not using otel-go net/http standard instrumentation support library?

@Harnoor7 Main reason is that I was not aware of it. But now that I've taken a first look at it, I think the library is missing some stuff (like metrics for the http client data, or keeping track of the readed bytes from the client). I might be wrong, and I will carefully review the differences there. The focus now is on having this PR in a usable state, then I will review and check we can have the same metrics / traces with the contrib library .. in that case I will use what I can from there, and if I miss something, we might think about trying to make a PR there.

I will keep an eye on it. Thanks for pointing out.

@Harnoor7
Copy link

Harnoor7 commented Feb 2, 2024

@dhontecillas Got it. Yes, you are correct client-side metric instrumentation is missing, which will be added probably in the next release. There is an active PR for it: open-telemetry/opentelemetry-go-contrib#4707

deepsource-autofix bot added a commit that referenced this pull request Feb 6, 2024
This commit fixes the style issues introduced in 652e8c9 according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Feb 9, 2024
This commit fixes the style issues introduced in ac63bfb according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Feb 12, 2024
This commit fixes the style issues introduced in 16e2532 according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Feb 12, 2024
This commit fixes the style issues introduced in 5a781c8 according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Feb 19, 2024
This commit fixes the style issues introduced in efb379c according to the output
from Go fmt and Gofumpt.

Details: #1
deepsource-autofix bot added a commit that referenced this pull request Feb 21, 2024
This commit fixes the style issues introduced in 54260ca according to the output
from Go fmt and Gofumpt.

Details: #1
@dhontecillas dhontecillas marked this pull request as ready for review February 23, 2024 15:52
@alombarte alombarte merged commit 10b64c0 into main Mar 5, 2024
2 checks passed
@alombarte alombarte deleted the initial_implementation branch March 5, 2024 08:20
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 this pull request may close these issues.

None yet

5 participants