Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@derickr derickr commented Nov 22, 2016

No description provided.

@jmikola jmikola mentioned this pull request

Jan 5, 2017

Logging support #304

Closed

@derickr derickr force-pushed the PHPC-349-apm branch from 3b32820 to 5dccaec Compare

January 12, 2017 11:59

@derickr derickr force-pushed the PHPC-349-apm branch 2 times, most recently from 5c84933 to 6f039a1 Compare

February 13, 2017 18:37

@derickr derickr force-pushed the PHPC-349-apm branch from 6f039a1 to 82a6a19 Compare

February 28, 2017 11:04

@derickr derickr force-pushed the PHPC-349-apm branch 8 times, most recently from c781336 to 5c02997 Compare

March 27, 2017 14:41

derickr added 2 commits

March 27, 2017 15:52


 PHPC-349 : Implement APM specification

57a75bd


 For failed tests, show the full .exp and .out files

bd76843

It only does this if there are failed tests, but XFAIL is not included in the
"failed tests".

@derickr derickr force-pushed the PHPC-349-apm branch from 5c02997 to bd76843 Compare

March 27, 2017 14:53

@derickr derickr changed the title ~~(削除) [WIP] PHPC-349: Implement APM specification (削除ここまで)~~ (追記) PHPC-349: Implement APM specification (追記ここまで)

Mar 27, 2017

jmikola

jmikola reviewed

View reviewed changes

php_phongo.c Outdated

static void php_phongo_command_started(const mongoc_apm_command_started_t *event)

{

mongoc_client_t *client = mongoc_apm_command_started_get_context(event);

Copy link

Member

Choose a reason for hiding this comment

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

Should function calls and statements appear after variable declarations in the same scope? IIRC, this is why TSRMLS_FETCH() can't precede declarations.

Copy link

Contributor Author

Choose a reason for hiding this comment

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

Not needed as far as I know. Our compile flags also create a warning if we do this wrong (through -Werror=declaration-after-statement).

From what I understand, you can define a variable and initialise this, but C90 does not allow to have "just code" before further variable declarations/initialisations.

jmikola added 8 commits

March 28, 2017 12:37


 Alphabetize lists of Monitoring classes

65547be


 Refactor php_phongo_dispatch_handlers() to a single #if block

938be3d


 Remove local variable in APM callbacks

a30d4dd


 Use ZEND_STRL() macro in php_phongo_command_failed()

3317e67


 Fix typo

3ed0ebf


 Remove redundant lines in Manager.c

06206fc


 PHP_FUNCTION is defined in php.h

daa5fc7


 Reformat MongoDB\Driver\Monitoring classes

4e6ba1f

jmikola

jmikola requested changes

View reviewed changes

config.m4 Outdated

src/MongoDB/Monitoring/CommandSubscriber.c \

src/MongoDB/Monitoring/CommandStartedEvent.c \

src/MongoDB/Monitoring/CommandSucceededEvent.c \

src/MongoDB/Monitoring/CommandFailedEvent.c

Copy link

Member

Choose a reason for hiding this comment

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

Can we alphabetize these for consistency?

Copy link

Contributor Author

Choose a reason for hiding this comment

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

Yes - although, they're now ordered logically.

Copy link

Member

Choose a reason for hiding this comment

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

I did keep the logical order when initializing the class entries in #568.

config.w32 Outdated

ADD_SOURCES(configure_module_dirname + "/src/MongoDB", "BulkWrite.c Command.c Cursor.c CursorId.c Manager.c Query.c ReadConcern.c ReadPreference.c Server.c WriteConcern.c WriteConcernError.c WriteError.c WriteResult.c", "mongodb");

ADD_SOURCES(configure_module_dirname + "/src/MongoDB", "BulkWrite.c Command.c Cursor.c CursorId.c Manager.c Monitoring.c Query.c ReadConcern.c ReadPreference.c Server.c WriteConcern.c WriteConcernError.c WriteError.c WriteResult.c", "mongodb");

ADD_SOURCES(configure_module_dirname + "/src/MongoDB/Exception", "AuthenticationException.c BulkWriteException.c ConnectionException.c ConnectionTimeoutException.c Exception.c ExecutionTimeoutException.c InvalidArgumentException.c LogicException.c RuntimeException.c SSLConnectionException.c UnexpectedValueException.c WriteException.c", "mongodb");

ADD_SOURCES(configure_module_dirname + "/src/MongoDB/Monitoring", "Subscriber.c CommandSubscriber.c CommandStartedEvent.c CommandSucceededEvent.c CommandFailedEvent.c", "mongodb");

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

Can we alphabetize these for consistency?

Copy link

Contributor Author

@derickr derickr Mar 29, 2017

Choose a reason for hiding this comment

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

Yes, although they're now ordered logically.

php_phongo.c Outdated

static void php_phongo_command_succeeded(const mongoc_apm_command_succeeded_t *event)

{

mongoc_client_t *client = mongoc_apm_command_succeeded_get_context(event);

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

This is only used once in this function. Why not remove the local variable and relocate mongoc_apm_command_succeeded_get_context() to the p_event->client assignment below?

Copy link

Contributor Author

@derickr derickr Mar 29, 2017

Choose a reason for hiding this comment

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

php_phongo.c

#else

zval *z_event = NULL;

#endif

TSRMLS_FETCH();

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

Since TSRMLS_FETCH() is defined as nothing in PHP 7+, would it make sense to combine the declarations and iteration so this function has a single #if block?

php_phongo.c Outdated

static void php_phongo_command_failed(const mongoc_apm_command_failed_t *event)

{

mongoc_client_t *client = mongoc_apm_command_failed_get_context(event);

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

This local can also be removed by inlining this in the p_event->client assignment.

Copy link

Contributor Author

@derickr derickr Mar 29, 2017

Choose a reason for hiding this comment

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

src/MongoDB/Manager.c Outdated

* Operation methods do not take socket-level options (e.g. socketTimeoutMS).

* Those options should be specified during construction.

/* {{{ MongoDB\Driver\Manager */

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

This appears to be inadvertently added.

Copy link

Contributor Author

@derickr derickr Mar 29, 2017

Choose a reason for hiding this comment

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

It's not new code, probably comes from a mess while rebasing.

src/MongoDB/Manager.c Outdated

phongo_throw_exception(PHONGO_ERROR_RUNTIME TSRMLS_CC, "%s", "MongoDB\\Driver objects cannot be serialized");

}

/* }}} */

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

This is unused, as the function entry uses MongoDB_disabled___wakeup.

Copy link

Contributor Author

@derickr derickr Mar 29, 2017

Choose a reason for hiding this comment

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

It's not new code, probably comes from a mess while rebasing.

src/MongoDB/Manager.c Outdated

#define PHONGO_MANAGER_URI_DEFAULT "mongodb://127.0.0.1/"

ZEND_EXTERN_MODULE_GLOBALS(mongodb)

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

This doesn't appear to be necessary.

Copy link

Contributor Author

@derickr derickr Mar 29, 2017

Choose a reason for hiding this comment

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

It was definitely needed when I wrote the code. If any code in this file uses MONGODB_G, it needs to be there. Seems that we removed all of that code though.

src/MongoDB/Manager.c

if (intern->client) {

php_phongo_set_monitoring_callbacks(intern->client);

}

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

Why is this done here instead of phongo_manager_init()? Also, if the client was persisted from a previous request, shouldn't the callbacks already be applied?

Copy link

Contributor Author

@derickr derickr Mar 29, 2017

Choose a reason for hiding this comment

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

Just as my comment on the HHVM PR, in earlier discussions we came to the conclusion that we didn't want to remove the apm callbacks at request shutdown, and to be safe, always set them upon Manager creation.

Copy link

Member

@jmikola jmikola Mar 29, 2017

Choose a reason for hiding this comment

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

In that case, I think we can still move this to phongo_manager_init() just to keep this logic out of the constructor. I'd like to revisit down the line with some testing over multiple requests to see if re-assignment is really necessary. Since the callbacks are fixed functions, I have a feeling this has no effect.

src/MongoDB/Monitoring.c

#if PHP_VERSION_ID >= 70000

zend_hash_str_del(&MONGODB_G(subscribers), hash, strlen(hash));

#else

zend_hash_del(&MONGODB_G(subscribers), hash, strlen(hash));

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

If zend_hash_update() doesn't add a reference, are you sure that this removes a reference on the zval after deleting it from the hash?

Copy link

Member

@jmikola jmikola Mar 28, 2017

Choose a reason for hiding this comment

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

I suppose if there was a leak, the removeSubscriber() tests would catch it. I found no issues running Valgrind locally for 5.6-zts and 7.1.

Copy link

Contributor Author

@derickr derickr Mar 29, 2017

Choose a reason for hiding this comment

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

No, as the hash dtor uses zval_ptr_dtor on the element, it shouldn't be needed. AFAIK, the dtor decreases the refcount and frees when the refcount hits 0.

@jmikola jmikola mentioned this pull request

PHPC-349: Fixes and code cleanup #568

Merged

Copy link

Member

jmikola commented Mar 28, 2017

Most of the minor comments (e.g. formatting, alphabetizing) are addressed in #568.

@jmikola jmikola mentioned this pull request

HHVM-190: Implement APM Specification mongodb/mongo-hhvm-driver#173

Merged

jmikola and others added 3 commits

March 30, 2017 09:53


 Use static decl for phongo_writeresult_init()

cab4e9b


 Use uint32_t for server_id in APM structs

e8cdf3e


 Merge branch 'pr/568' into PHPC-349 -apm

0bb78c1

derickr added a commit that referenced this pull request

Mar 31, 2017