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.

Sign up for GitHub

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

Jump to bottom

Elasticsearch integration #464

Merged
merged 9 commits into from
Feb 24, 2023
Merged

Elasticsearch integration #464

merged 9 commits into from
Feb 24, 2023

Conversation

esmegl
Copy link
Contributor

@esmegl esmegl commented Feb 19, 2023 •
edited
Loading

  • Added elasticsearch configurations on dockering folder
  • Added --es command on piker/cli/__init__.py
  • Created elasticsearch script to start elasticsearch instance
  • Modified ahab to support elasticsearch logs and init procedure
  • Created new tests for elasticsearch and markestore initialization

guilledk
guilledk reviewed Feb 20, 2023
View reviewed changes
piker/cli/__init__.py Outdated Show resolved Hide resolved
goodboy
goodboy reviewed Feb 20, 2023
View reviewed changes
piker/data/_ahab.py Outdated Show resolved Hide resolved
goodboy
goodboy reviewed Feb 20, 2023
View reviewed changes
piker/data/_ahab.py Outdated Show resolved Hide resolved
goodboy
goodboy reviewed Feb 20, 2023
View reviewed changes
piker/data/_ahab.py Outdated Show resolved Hide resolved
goodboy
goodboy reviewed Feb 20, 2023
View reviewed changes
piker/data/marketstore.py Outdated Show resolved Hide resolved
goodboy
goodboy approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

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

Other then some slight nitpicks this looks good to me.

I'll let @guilledk guide the patch set to completion / merge.

I'd like to also give this at least a serious manual test (once you flip it to a non-draft PR) before landing, though if y'all feel comfortable adding a small integration test (maybe to test_services.py?) then that would be even better 🏄🏼

@guilledk guilledk force-pushed the elasticsearch_integration branch 6 times, most recently from 9875537 to 08bacad Compare February 21, 2023 17:57
@goodboy goodboy mentioned this pull request Feb 21, 2023
Merged
3 tasks
@guilledk guilledk force-pushed the elasticsearch_integration branch 6 times, most recently from d2898b9 to e519fe7 Compare February 21, 2023 23:39
@guilledk
Remove unnesesary arguments to some pikerd functions, fix container i…
acc6249 
…nit error

by switching from log reading to quering es health endpoint, fix install on ci
and add more logging.
@guilledk guilledk force-pushed the elasticsearch_integration branch from e519fe7 to acc6249 Compare February 21, 2023 23:45
@esmegl esmegl marked this pull request as ready for review February 22, 2023 16:33
guilledk
guilledk approved these changes Feb 22, 2023
View reviewed changes
piker/data/_ahab.py Show resolved Hide resolved
@guilledk guilledk requested a review from goodboy February 22, 2023 17:07
goodboy
goodboy reviewed Feb 22, 2023
View reviewed changes
tests/test_databases.py Outdated Show resolved Hide resolved
goodboy
goodboy reviewed Feb 22, 2023
View reviewed changes
tests/test_databases.py Outdated Show resolved Hide resolved
goodboy
goodboy reviewed Feb 22, 2023
View reviewed changes
tests/test_databases.py Outdated Show resolved Hide resolved
goodboy
goodboy reviewed Feb 22, 2023
View reviewed changes
piker/data/elastic.py Outdated Show resolved Hide resolved
goodboy
goodboy reviewed Feb 22, 2023
View reviewed changes
piker/data/elastic.py
dcntr,
{},
# expected startup and stop msgs
start_matcher,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm these are really more like generic predicate functions now instead of just being msg matchers. I wonder the naming will confuse anyone going foward? Maybe it's not imporant for now 😂

goodboy
goodboy reviewed Feb 22, 2023
View reviewed changes
piker/data/elastic.py Outdated Show resolved Hide resolved
goodboy
goodboy reviewed Feb 22, 2023
View reviewed changes
piker/data/_ahab.py Outdated Show resolved Hide resolved
goodboy
goodboy requested changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

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

@esmegl lookin super duper good!

I have a couple nitpicks mostly naming and style related and then I think this is definitely ready to land 😎

goodboy
goodboy reviewed Feb 24, 2023
View reviewed changes
piker/data/_ahab.py
patt: str,
# this is a predicate func for matching log msgs emitted by the
# underlying containerized app
patt_matcher: Callable[[str], bool],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the typing here is techincally an Awaitable now, but it's not a big deal and i don't want this to delay the patch.

goodboy
goodboy reviewed Feb 24, 2023
View reviewed changes
piker/data/elastic.py
log.info(health)
return health[0]['status'] == 'green'

async def stop_matcher(msg: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

returns a bool. again though, not a huge deal and shouldn't block this.

goodboy
goodboy reviewed Feb 24, 2023
View reviewed changes
tests/test_databases.py
open_test_pikerd: AsyncContextManager,
loglevel,
):

Copy link
Contributor

Choose a reason for hiding this comment

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

don't need this newline

goodboy
goodboy approved these changes Feb 24, 2023
View reviewed changes
Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

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

A couple more minor suggestions but nothing that should necessarily block this if y'all are happy with it 👍🏼

@guilledk @esmegl great work yet again and nice to see a new integration!

@guilledk guilledk merged commit 47bf45f into master Feb 24, 2023
@guilledk guilledk deleted the elasticsearch_integration branch February 24, 2023 19:38
goodboy added a commit that referenced this pull request Mar 8, 2023
@goodboy
Support startup-config overrides to ahabd super
d3272ed 
With the addition of a new `elastixsearch` docker support in
#464, adjustments were made
to container startup sync logic (particularly the `trio` checkpoint
sleep period - which itself is a hack around a sync client api) which
caused a regression in upstream startup logic wherein container error
logs were not being bubbled up correctly causing a silent failure mode:

- `marketstore` container started with corrupt input config
- `ahabd` super code timed out on startup phase due to a larger log
  polling period, skipped processing startup logs from the container,
  and continued on as though the container was started
- history client fails on grpc connection with no clear error on why the
  connection failed.

Here we revert to the old poll period (1ms) to avoid any more silent
failures and further extend supervisor control through a configuration
override mechanism. To address the underlying design issue, this patch
adds support for container-endpoint-callbacks to override supervisor
startup configuration parameters via the 2nd value in their returned
tuple: the already delivered configuration `dict` value.

The current exposed values include:
    {
        'startup_timeout': 1.0,
        'startup_query_period': 0.001,
        'log_msg_key': 'msg',
    },

This allows for container specific control over the startup-sync query
period (the hack mentioned above)  as well as the expected log msg key
and of course the startup timeout.
goodboy added a commit that referenced this pull request Mar 8, 2023
@goodboy
Support startup-config overrides to ahabd super
1f1b231 
With the addition of a new `elastixsearch` docker support in
#464, adjustments were made
to container startup sync logic (particularly the `trio` checkpoint
sleep period - which itself is a hack around a sync client api) which
caused a regression in upstream startup logic wherein container error
logs were not being bubbled up correctly causing a silent failure mode:

- `marketstore` container started with corrupt input config
- `ahabd` super code timed out on startup phase due to a larger log
  polling period, skipped processing startup logs from the container,
  and continued on as though the container was started
- history client fails on grpc connection with no clear error on why the
  connection failed.

Here we revert to the old poll period (1ms) to avoid any more silent
failures and further extend supervisor control through a configuration
override mechanism. To address the underlying design issue, this patch
adds support for container-endpoint-callbacks to override supervisor
startup configuration parameters via the 2nd value in their returned
tuple: the already delivered configuration `dict` value.

The current exposed values include:
    {
        'startup_timeout': 1.0,
        'startup_query_period': 0.001,
        'log_msg_key': 'msg',
    },

This allows for container specific control over the startup-sync query
period (the hack mentioned above)  as well as the expected log msg key
and of course the startup timeout.
goodboy added a commit that referenced this pull request Mar 9, 2023
@goodboy
Support startup-config overrides to ahabd super
7b196b1 
With the addition of a new `elastixsearch` docker support in
#464, adjustments were made
to container startup sync logic (particularly the `trio` checkpoint
sleep period - which itself is a hack around a sync client api) which
caused a regression in upstream startup logic wherein container error
logs were not being bubbled up correctly causing a silent failure mode:

- `marketstore` container started with corrupt input config
- `ahabd` super code timed out on startup phase due to a larger log
  polling period, skipped processing startup logs from the container,
  and continued on as though the container was started
- history client fails on grpc connection with no clear error on why the
  connection failed.

Here we revert to the old poll period (1ms) to avoid any more silent
failures and further extend supervisor control through a configuration
override mechanism. To address the underlying design issue, this patch
adds support for container-endpoint-callbacks to override supervisor
startup configuration parameters via the 2nd value in their returned
tuple: the already delivered configuration `dict` value.

The current exposed values include:
    {
        'startup_timeout': 1.0,
        'startup_query_period': 0.001,
        'log_msg_key': 'msg',
    },

This allows for container specific control over the startup-sync query
period (the hack mentioned above)  as well as the expected log msg key
and of course the startup timeout.
@goodboy goodboy mentioned this pull request Mar 10, 2023
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guilledk guilledk guilledk approved these changes

@goodboy goodboy goodboy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@esmegl @goodboy @guilledk