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

T2.3 decission support tool #179

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

JBenda
Copy link
Contributor

@JBenda JBenda commented Mar 14, 2023

Adding decision support to JULEA.

New:

  • trace level access which logs server backend operations.
  • tool access-replay which takes a JULEA_TRACE=access trace and replays the backend operations of it
  • scripts/decission-support.py runs access-replay multiple times for different configurations described in a config file
  • scripts/decission-support.R converts the output of a scripts/decission-support.py to plots and tables, which can be viewed as text or webpage
  • scripts/style.css and scripts/interactive.js for interactive and nice HTML page, where to put this file properly?

Further documentation can be found in doc/decission-support.md

Additional analytics possible on this data, but not yet implemented:

  • Cardinality of objects access (regarding different processes)
  • read/write ratio for different object categories
  • analytics regarding access type clusters and frequency

Added build option julea_trace to enable tracing also for release builds

@@ -317,15 +317,22 @@ j_bson_iter_value(bson_iter_t* iter, JDBType type, JDBTypeValue* value, GError**

break;
case J_DB_TYPE_SINT64:
if (G_UNLIKELY(!BSON_ITER_HOLDS_INT64(iter)))
if (G_UNLIKELY(!BSON_ITER_HOLDS_INT64(iter) && !BSON_ITER_HOLDS_INT32(iter)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bson containing int32 should also be able to be parsed

if (client_program_name == NULL)
{
client_program_name = "<unkown>";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Different ways to get a program name, if there is a better way, please let me know.

lib/core/jtrace.c Outdated Show resolved Hide resolved
}

static guint32
count_keys_recursive(const bson_t* bson)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Number of keys in query bson ↔ complexity measure

lib/core/jtrace.c Outdated Show resolved Hide resolved
}
else
{
/// \TODO handle unknown backends
Copy link
Contributor Author

Choose a reason for hiding this comment

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

warning? panic?

Copy link
Contributor

Choose a reason for hiding this comment

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

warning imo

ret = j_backend_db_schema_get(db_backend, batch, parts[NAME], schema, &error);
g_array_append_val(bsons, schema);
// schema do not exists
if (error && error->code == 8)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normal behavior is to check if i schema exists before creating a new one, therefore a schema does not exist is no error to panic

else if (strncmp(name, "backend_", 8) == 0)
{
int type = 0;
if (trace_thread->access.db.type == NULL) /// \TODO more precise checking?
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by precise checking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is for loading the type and config_path of each backend, which cannot be done in the init since the trace will be initialized before the backends.

Instead of checking db.type, it could be checked more of null, but this would mean an invalid state .... so think the test is enough, and will remove the todo

trace_thread->client.uid = va_arg(args, guint32);
va_end(args);
}
else if (strncmp(name, "backend_", 8) == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a comment here to clarify that this line filters for trace calls with backend_ prefix.

name += 8;
if (strncmp(name, "kv_", 3) == 0)
{
type = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

those 'magic' numbers should probably be an enum for readability.


j_configuration_unref(config);
}
name += 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hard to catch should the prefix change later on.
Maybe add defines for the prefix and prefix length? This would still avoid pattern matching overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

also used later on: is it feasible to split the string (even before the match against 'backend_') on '_' and compare parts instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot split at every _ because sometimes it appears in the name, like get_all. And doing it before the check for backend_ seems like a lot of potential string comparing and allocating because the name is constant. Therefore, we cannot simply add \0

lib/core/jtrace.c Outdated Show resolved Hide resolved
lib/core/jtrace.c Outdated Show resolved Hide resolved
lib/core/jtrace.c Outdated Show resolved Hide resolved
lib/core/jtrace.c Outdated Show resolved Hide resolved
goto end;
}

// j_trace_init("julea-replay");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be out-commented? Otherwise it is probably better to delete

};

static gboolean
split(char* line, char* parts[ROW_LEN])
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also g_strsplit but I think I like your version better :)

char* line = NULL;
size_t len = 0;
ssize_t read = 0;
memset(memory_chunk, 0, memory_chunck_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should memory_chunk be initialized with random values instead of only 0?

g_warning("unkown operation: '%s'", op);
}
return ret;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some empty lines for readability :)

tools/access_replay.c Outdated Show resolved Hide resolved
@t-erxleben
Copy link
Contributor

Does it make sense to enfore JULEA_TRACE=access in julea-access-replay so the user does not have to care to set it manually? It seems like this would be the default use case for the tool.
Also: are echo and access compatible? (e.g., what happens when both are specified, since they both output to stderr).

@t-erxleben
Copy link
Contributor

I rewrote the doc file here.
Feel free to cherry-pick and/or argue about changes I made :)

@JBenda
Copy link
Contributor Author

JBenda commented Mar 20, 2023

Does it make sense to enfore JULEA_TRACE=access in julea-access-replay so the user does not have to care to set it manually? It seems like this would be the default use case for the tool. Also: are echo and access compatible? (e.g., what happens when both are specified, since they both output to stderr).

You could also use replays to test a configuration for its functionality. So, you do not need to run an expensive program or made up integration test, just re-run the data access. Also reduces dependencies for such cases.

Since each echo line starts with [timestamp] it would be easy to filter out these lines. For debugging reasons (e.g. check if the access records are correct) it would make sens to see both in chronologically order.

@t-erxleben
Copy link
Contributor

Does it make sense to enfore JULEA_TRACE=access in julea-access-replay so the user does not have to care to set it manually? It seems like this would be the default use case for the tool. Also: are echo and access compatible? (e.g., what happens when both are specified, since they both output to stderr).

You could also use replays to test a configuration for its functionality. So, you do not need to run an expensive program or made up integration test, just re-run the data access. Also reduces dependencies for such cases.

Since each echo line starts with [timestamp] it would be easy to filter out these lines. For debugging reasons (e.g. check if the access records are correct) it would make sens to see both in chronologically order.

Alright :)

@t-erxleben
Copy link
Contributor

I get warnings during the build:

➜ ninja -C bld
ninja: Entering directory `bld'
[24/176] Compiling C object libjulea.so.p/lib_core_jtrace.c.o
../lib/core/jtrace.c: In function ‘parse_backend_operation’:
../lib/core/jtrace.c:645:1: warning: function returns an aggregate [-Waggregate-return]
  645 | parse_backend_operation(const gchar* backend_operation)
      | ^~~~~~~~~~~~~~~~~~~~~~~
../lib/core/jtrace.c: In function ‘j_trace_enter’:
../lib/core/jtrace.c:753:32: warning: function call has aggregate value [-Waggregate-return]
  753 |                         type = parse_backend_operation(backend_operation);
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/core/jtrace.c: In function ‘j_trace_leave’:
../lib/core/jtrace.c:1075:61: warning: function call has aggregate value [-Waggregate-return]
 1075 |                                 BackendTypeOperation type = parse_backend_operation(backend_operation);
      |                                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[176/176] Linking target julea-fuse

@t-erxleben
Copy link
Contributor

I worked through the examples given in the documentation and everything worked fine.

As an addition it could be nice to mention the R package dependencies in the docs (probably as a command that install them?). I ran through a few iterations of trying to execute the R script and installing missing packages.

@JBenda
Copy link
Contributor Author

JBenda commented Mar 22, 2023

I get warnings during the build:

➜ ninja -C bld
ninja: Entering directory `bld'
[24/176] Compiling C object libjulea.so.p/lib_core_jtrace.c.o
../lib/core/jtrace.c: In function ‘parse_backend_operation’:
../lib/core/jtrace.c:645:1: warning: function returns an aggregate [-Waggregate-return]
  645 | parse_backend_operation(const gchar* backend_operation)
      | ^~~~~~~~~~~~~~~~~~~~~~~
../lib/core/jtrace.c: In function ‘j_trace_enter’:
../lib/core/jtrace.c:753:32: warning: function call has aggregate value [-Waggregate-return]
  753 |                         type = parse_backend_operation(backend_operation);
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/core/jtrace.c: In function ‘j_trace_leave’:
../lib/core/jtrace.c:1075:61: warning: function call has aggregate value [-Waggregate-return]
 1075 |                                 BackendTypeOperation type = parse_backend_operation(backend_operation);
      |                                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[176/176] Linking target julea-fuse

This is fixed now.

@JBenda JBenda force-pushed the feature/t23-decission-support-tool branch from 534bee5 to 7839ad2 Compare March 26, 2023 12:07
@JBenda JBenda force-pushed the feature/t23-decission-support-tool branch from ab1f9c6 to 6921602 Compare May 23, 2023 11:23
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.

2 participants