Skip to content

feat: refactor core module into multiple packages#530

Merged
beeme1mr merged 6 commits intoopen-feature:mainfrom
thisthat:feat/501/expose-impressions
Mar 22, 2023
Merged

feat: refactor core module into multiple packages#530
beeme1mr merged 6 commits intoopen-feature:mainfrom
thisthat:feat/501/expose-impressions

Conversation

@thisthat
Copy link
Copy Markdown
Member

@thisthat thisthat commented Mar 17, 2023

Signed-off-by: Giovanni Liva giovanni.liva@dynatrace.com

This PR

  • Refactor the service_[connect|metrics].go into multiple files
  • Adds more unit tests when easy and feasible

Related Issues

Preparation for #501

@toddbaert
Copy link
Copy Markdown
Member

I like this refactor. In general it's my preference to see refactors done in separate PRs. It's helps to be able to approach a refactor PR knowing there's no functional changes. I'd like to see it separated if possible, but it's not necessary.

@thisthat
Copy link
Copy Markdown
Member Author

I like this refactor. In general it's my preference to see refactors done in separate PRs. It's helps to be able to approach a refactor PR knowing there's no functional changes. I'd like to see it separated if possible, but it's not necessary.

That's a fair argument. I will remove the impressions and just tackle the refactoring with this PR 👍

Comment thread core/pkg/service/flag_evaluation.go
Comment thread core/pkg/otel/metrics.go Outdated
Comment thread core/pkg/otel/metrics.go
Comment thread core/pkg/otel/metrics.go
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 20, 2023

Codecov Report

Merging #530 (7e44481) into main (99ba5ec) will increase coverage by 0.50%.
The diff coverage is 71.77%.

@@            Coverage Diff             @@
##             main     #530      +/-   ##
==========================================
+ Coverage   66.16%   66.66%   +0.50%     
==========================================
  Files          17       19       +2     
  Lines        2113     2133      +20     
==========================================
+ Hits         1398     1422      +24     
  Misses        651      651              
+ Partials       64       60       -4     
Impacted Files Coverage Δ
core/pkg/runtime/from_config.go 26.90% <0.00%> (-1.15%) ⬇️
core/pkg/runtime/runtime.go 0.00% <0.00%> (ø)
...kg/service/flag-evaluation/flag_evaluator_types.go 89.65% <ø> (ø)
core/pkg/service/flag-evaluation/flag_evaluator.go 61.81% <61.81%> (ø)
core/pkg/service/middleware/http_metrics.go 76.25% <76.25%> (ø)
...ore/pkg/service/flag-evaluation/connect_service.go 63.63% <94.11%> (+3.77%) ⬆️
core/pkg/otel/metrics.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment thread core/pkg/otel/metrics_test.go Outdated
@thisthat thisthat force-pushed the feat/501/expose-impressions branch from 71708b0 to 126dd39 Compare March 20, 2023 14:56
@thisthat thisthat changed the title feat: expose impressions as OTel metrics feat: refactor core module into multiple packages Mar 20, 2023
@thisthat
Copy link
Copy Markdown
Member Author

I need some help with the integration-tests @toddbaert
Locally, they also don't work. The strange thing is that I can reach the binary under test via curl curl -X POST "localhost:8013/schema.v1.Service/ResolveString" -d '{"flagKey":"string-flag","context":{}}' -H "Content-Type: application/json"

@thisthat thisthat marked this pull request as ready for review March 20, 2023 16:34
@thisthat thisthat force-pushed the feat/501/expose-impressions branch 2 times, most recently from 7930075 to 70f00d3 Compare March 21, 2023 12:07
@thisthat thisthat marked this pull request as draft March 21, 2023 13:13
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
@thisthat thisthat force-pushed the feat/501/expose-impressions branch from 70f00d3 to 9297e0b Compare March 21, 2023 13:57
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
@thisthat thisthat marked this pull request as ready for review March 21, 2023 15:25
@thisthat
Copy link
Copy Markdown
Member Author

thisthat commented Mar 21, 2023

I need some help with the integration-tests @toddbaert Locally, they also don't work. The strange thing is that I can reach the binary under test via curl curl -X POST "localhost:8013/schema.v1.Service/ResolveString" -d '{"flagKey":"string-flag","context":{}}' -H "Content-Type: application/json"

For the record, connect-go requires an implementation of Flush() to work properly. Thanks to @bacherfl to figure it out

Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
@thisthat thisthat requested a review from toddbaert March 22, 2023 06:53
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
@thisthat thisthat requested review from bacherfl and removed request for toddbaert March 22, 2023 07:20
Copy link
Copy Markdown
Contributor

@bacherfl bacherfl left a comment

Choose a reason for hiding this comment

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

lgtm

Comment thread core/pkg/service/connect_service.go Outdated
Comment thread core/pkg/service/middleware/http_metrics.go
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
@toddbaert toddbaert self-requested a review March 22, 2023 15:23
@beeme1mr beeme1mr self-requested a review March 22, 2023 19:01
@beeme1mr beeme1mr merged commit 9d68d0b into open-feature:main Mar 22, 2023
@github-actions github-actions Bot mentioned this pull request Mar 22, 2023
toddbaert added a commit that referenced this pull request Mar 30, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>flagd: 0.5.0</summary>

##
[0.5.0](flagd/v0.4.5...flagd/v0.5.0)
(2023-03-30)


### ⚠ BREAKING CHANGES

* unify sources configuration handling
([#560](#560))

### 🐛 Bug Fixes

* benchmark pipeline
([#538](#538))
([62cc0fc](62cc0fc))
* **deps:** update module github.com/open-feature/flagd/core to v0.4.5
([#552](#552))
([41799f6](41799f6))


### 🧹 Chore

* refactor configuration handling for startup
([#551](#551))
([8dfbde5](8dfbde5))
</details>

<details><summary>flagd-proxy: 0.2.0</summary>

##
[0.2.0](flagd-proxy-v0.1.2...flagd-proxy/v0.2.0)
(2023-03-30)


### ⚠ BREAKING CHANGES

* rename `kube-flagd-proxy` to `flagd-proxy`
([#576](#576))

### ✨ New Features

* rename `kube-flagd-proxy` to `flagd-proxy`
([#576](#576))
([223de99](223de99))
</details>

<details><summary>core: 0.5.0</summary>

##
[0.5.0](core/v0.4.5...core/v0.5.0)
(2023-03-30)


### ⚠ BREAKING CHANGES

* rename `kube-flagd-proxy` to `flagd-proxy`
([#576](#576))
* unify sources configuration handling
([#560](#560))

### 🧹 Chore

* move credential builder for grpc sync into seperate component
([#536](#536))
([7314fee](7314fee))
* refactor configuration handling for startup
([#551](#551))
([8dfbde5](8dfbde5))
* refactor middleware setup in server
([#554](#554))
([01016c7](01016c7))
* refactor service configuration objects
([#545](#545))
([c7b29ed](c7b29ed)),
closes [#524](#524)
* unify sources configuration handling
([#560](#560))
([7f4888a](7f4888a))


### 🐛 Bug Fixes

* **deps:** update module google.golang.org/grpc to v1.54.0
([#548](#548))
([99ba5ec](99ba5ec))
* **deps:** update module sigs.k8s.io/controller-runtime to v0.14.6
([#572](#572))
([bed9458](bed9458))
* fixing silent lint failures
([#550](#550))
([30c8022](30c8022))
* nil pointer fix + export constructors
([#555](#555))
([78adb81](78adb81))


### ✨ New Features

* expose Impression metric
([#556](#556))
([77e0a33](77e0a33))
* Introduce kube-proxy-metrics
([#558](#558))
([ad0baeb](ad0baeb))
* rename `kube-flagd-proxy` to `flagd-proxy`
([#576](#576))
([223de99](223de99))
* refactor core module into multiple packages
([#530](#530))
([9d68d0b](9d68d0b))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Signed-off-by: Todd Baert <toddbaert@gmail.com>
Co-authored-by: Todd Baert <toddbaert@gmail.com>
@github-actions github-actions Bot mentioned this pull request Dec 2, 2023
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.

4 participants